Ocfs2 devel - Sep 2015 - [PATCH] ocfs2: Fill-in the unused portion of the block with zeros by dio_zero_block()

A simplified test case is (this case from Ryan):
1) dd if=/dev/zero of=/mnt/hello bs=512 count=1 oflag=direct;
2) truncate /mnt/hello -s 2097152
file 'hello' is not exist before test. After this command,
file 'hello' should be all zero. But 512~4096 is some random data.

Setting bh state to new when get a new block, if so,
direct_io_worker()->dio_zero_block() will fill-in the unused portion
of the block with zero.

Signed-off-by: Yiwen Jiang <jiangyiwen at huawei.com>
---
 fs/ocfs2/aops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 1a35c61..bd106b9 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -581,6 +581,7 @@ static int ocfs2_direct_IO_get_blocks(struct inode *inode,
sector_t iblock,
 			ret = -EIO;
 			goto bail;
 		}
+		set_buffer_new(bh_result);
 	}

 	/*
-- 
1.8.3.4

On 2015/9/9 9:55, jiangyiwen wrote:
> A simplified test case is (this case from Ryan):
> 1) dd if=/dev/zero of=/mnt/hello bs=512 count=1 oflag=direct;
> 2) truncate /mnt/hello -s 2097152
> file 'hello' is not exist before test. After this command,
> file 'hello' should be all zero. But 512~4096 is some random data.
> 
> Setting bh state to new when get a new block, if so,
> direct_io_worker()->dio_zero_block() will fill-in the unused portion
> of the block with zero.
> 
> Signed-off-by: Yiwen Jiang <jiangyiwen at huawei.com>
Reviewed-by: Joseph Qi <joseph.qi at huawei.com>
> ---
>  fs/ocfs2/aops.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 1a35c61..bd106b9 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -581,6 +581,7 @@ static int ocfs2_direct_IO_get_blocks(struct inode
*inode, sector_t iblock,
>  			ret = -EIO;
>  			goto bail;
>  		}
> +		set_buffer_new(bh_result);
>  	}
> 
>  	/*
>

Hi Yiwen,

I try to reproduce this case, but it didn't act like you describe.
What I did:
==laptop:/mnt/shared # mount | grep ocfs2
ocfs2_dlmfs on /dlm type ocfs2_dlmfs (rw,relatime)
/dev/sda3 on /mnt/shared type ocfs2
(rw,relatime,_netdev,heartbeat=local,nointr,data=ordered,errors=remount-ro,atime_quantum=60,coherency=full,user_xattr,acl)
laptop:/mnt/shared # dd if=/dev/zero of=/mnt/shared/hello bs=512 count=1
oflag=direct
1+0 records in
1+0 records out
512 bytes (512 B) copied, 0.000684382 s, 748 kB/s
laptop:/mnt/shared # truncate hello -s 2097152
laptop:/mnt/shared # cat hello                            ======> nothing
laptop:/mnt/shared # uname -r
3.16.7-21-desktop
==
Did I do something wrong? If I misunderstand, please correct me.
Thanks.

 >>>
> A simplified test case is (this case from Ryan): 
> 1) dd if=/dev/zero of=/mnt/hello bs=512 count=1 oflag=direct; 
> 2) truncate /mnt/hello -s 2097152 
> file 'hello' is not exist before test. After this command, 
> file 'hello' should be all zero. But 512~4096 is some random data. 
>  
> Setting bh state to new when get a new block, if so, 
> direct_io_worker()->dio_zero_block() will fill-in the unused portion 
> of the block with zero. 
>  
> Signed-off-by: Yiwen Jiang <jiangyiwen at huawei.com> 
> --- 
>  fs/ocfs2/aops.c | 1 + 
>  1 file changed, 1 insertion(+) 
>  
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c 
> index 1a35c61..bd106b9 100644 
> --- a/fs/ocfs2/aops.c 
> +++ b/fs/ocfs2/aops.c 
> @@ -581,6 +581,7 @@ static int ocfs2_direct_IO_get_blocks(struct inode  
> *inode, sector_t iblock, 
>  			ret = -EIO; 
>  			goto bail; 
>  		} 
> +		set_buffer_new(bh_result); 
>  	} 
>  
>  	/* 
 



--
Eric, Ren

Hi Yiwen,

I'm working on this issue. The patch will be send out soon. And the 
issue that do not support file hole will be fixed too. I have proved it 
can pass all ltp-aiodio test cases, and has better performance. ;-)

Thanks,
Ryan


On 09/09/2015 09:55 AM, jiangyiwen wrote:
> A simplified test case is (this case from Ryan):
> 1) dd if=/dev/zero of=/mnt/hello bs=512 count=1 oflag=direct;
> 2) truncate /mnt/hello -s 2097152
> file 'hello' is not exist before test. After this command,
> file 'hello' should be all zero. But 512~4096 is some random data.
>
> Setting bh state to new when get a new block, if so,
> direct_io_worker()->dio_zero_block() will fill-in the unused portion
> of the block with zero.
>
> Signed-off-by: Yiwen Jiang <jiangyiwen at huawei.com>
> ---
>   fs/ocfs2/aops.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 1a35c61..bd106b9 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -581,6 +581,7 @@ static int ocfs2_direct_IO_get_blocks(struct inode
*inode, sector_t iblock,
>   			ret = -EIO;
>   			goto bail;
>   		}
> +		set_buffer_new(bh_result);
>   	}
>
>   	/*

On Wed, 9 Sep 2015 09:55:16 +0800 jiangyiwen <jiangyiwen at huawei.com>
wrote:
> A simplified test case is (this case from Ryan):
> 1) dd if=/dev/zero of=/mnt/hello bs=512 count=1 oflag=direct;
> 2) truncate /mnt/hello -s 2097152
> file 'hello' is not exist before test. After this command,
> file 'hello' should be all zero. But 512~4096 is some random data.
> 
> Setting bh state to new when get a new block, if so,
> direct_io_worker()->dio_zero_block() will fill-in the unused portion
> of the block with zero.
> 
> ...
>
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -581,6 +581,7 @@ static int ocfs2_direct_IO_get_blocks(struct inode
*inode, sector_t iblock,
>  			ret = -EIO;
>  			goto bail;
>  		}
> +		set_buffer_new(bh_result);
>  	}
> 
You're working against an old kernel.  I did this:

---
a/fs/ocfs2/aops.c~ocfs2-fill-in-the-unused-portion-of-the-block-with-zeros-by-dio_zero_block
+++ a/fs/ocfs2/aops.c
@@ -589,6 +589,7 @@ static int ocfs2_direct_IO_get_blocks(st
 			ret = -EIO;
 			goto bail;
 		}
+		set_buffer_new(bh_result);
 		up_write(&OCFS2_I(inode)->ip_alloc_sem);
 	}
 

Probably we could run set_buffer_new() after the up_write(), which
would decrease lok hold times a little bit.