ocfs2: Fill-in the unused portion of the block with zeros by dio_zero_block()
diff mbox

Message ID 55EF9184.5090007@huawei.com
State New
Headers show

Commit Message

jiangyiwen Sept. 9, 2015, 1:55 a.m. UTC
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@huawei.com>
---
 fs/ocfs2/aops.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Joseph Qi Sept. 9, 2015, 2:13 a.m. UTC | #1
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@huawei.com>
Reviewed-by: Joseph Qi <joseph.qi@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);
>  	}
> 
>  	/*
>
Zhen Ren Sept. 9, 2015, 5:16 a.m. UTC | #2
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@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
Joseph Qi Sept. 9, 2015, 9:13 a.m. UTC | #3
Hi Zhen,

On 2015/9/9 13:16, Zhen Ren wrote:
> 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
I don't think 'cat' works here.
Could you please use hexdump?

> 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@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
> 
> 
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
>
jiangyiwen Sept. 9, 2015, 9:14 a.m. UTC | #4
On 2015/9/9 13:16, Zhen Ren wrote:
> 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
Please use this command to read binary file "hexdump -C /mnt/shared/hello", you will
see the result like this:
linux-ZnXfWq:/mnt/ocfs2 # hexdump -C hello
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00200000
it means in file area 512~4096 is zero not random data.
> 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@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
> 
> 
> 
> 
> .
>
Zhen Ren Sept. 9, 2015, 10:42 a.m. UTC | #5
Hi Joseph and Yiwen,

Sorry for that misoperation. 

This time, I did the same cmds using "hexdump" on ocfs2 and btrfs.
The testing results look the same:
==
laptop:/mnt/shared # dd if=/dev/zero of=hello bs=512 count=1 oflag=direct                                    ===> on ocfs2
1+0 records in
1+0 records out
512 bytes (512 B) copied, 0.000834866 s, 613 kB/s
laptop:/mnt/shared # hexdump hello -C
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000200
laptop:/mnt/shared # truncate hello -s 2097152
laptop:/mnt/shared # truncate hello -s 2097152
laptop:/mnt/shared # hexdump hello -C
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00200000

laptop:~ # dd if=/dev/zero of=hello bs=512 count=1 oflag=direct                                                                         ===> on btrfs
1+0 records in
1+0 records out
512 bytes (512 B) copied, 0.000201759 s, 2.5 MB/s
laptop:~ # hexdump hello -C
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000200
laptop:~ # truncate hello -s 2097152
laptop:~ # hexdump hello -C
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00200000
==

BTW, Yiwen, I'm confused by your comment and commit message.
1. >> file 'hello' should be all zero. But 512~4096 is some random data. 
 2. it means in file area 512~4096 is zero not random data.

Anyway, I didn't look into this case, but just try to understand it. 
So, if there's any problem, let it go ;-)

Thanks.
--
Eric Ren

>>> Joseph Qi <joseph.qi@huawei.com> 09/09/15 6:04 PM >>>
Hi Zhen,

On 2015/9/9 13:16, Zhen Ren wrote:
> 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
I don't think 'cat' works here.
Could you please use hexdump?

> 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@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
> 
> 
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
>
jiangyiwen Sept. 10, 2015, 1:48 a.m. UTC | #6
On 2015/9/9 18:42, Zhen Ren wrote:
> Hi Joseph and Yiwen,
> 
> Sorry for that misoperation. 
> 
> This time, I did the same cmds using "hexdump" on ocfs2 and btrfs.
> The testing results look the same:
> ==
> laptop:/mnt/shared # dd if=/dev/zero of=hello bs=512 count=1 oflag=direct                                    ===> on ocfs2
> 1+0 records in
> 1+0 records out
> 512 bytes (512 B) copied, 0.000834866 s, 613 kB/s
> laptop:/mnt/shared # hexdump hello -C
> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 00000200
> laptop:/mnt/shared # truncate hello -s 2097152
> laptop:/mnt/shared # truncate hello -s 2097152
> laptop:/mnt/shared # hexdump hello -C
> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 00200000
> 
> laptop:~ # dd if=/dev/zero of=hello bs=512 count=1 oflag=direct                                                                         ===> on btrfs
> 1+0 records in
> 1+0 records out
> 512 bytes (512 B) copied, 0.000201759 s, 2.5 MB/s
> laptop:~ # hexdump hello -C
> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 00000200
> laptop:~ # truncate hello -s 2097152
> laptop:~ # hexdump hello -C
> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 00200000
> ==
> 
> BTW, Yiwen, I'm confused by your comment and commit message.
> 1. >> file 'hello' should be all zero. But 512~4096 is some random data. 
>  2. it means in file area 512~4096 is zero not random data.
> 
> Anyway, I didn't look into this case, but just try to understand it. 
> So, if there's any problem, let it go ;-)
sorry, my description is wrong, 512~4096 is zero.

Thanks,
Yiwen Jiang
> 
> Thanks.
> --
> Eric Ren
> 
>>>> Joseph Qi <joseph.qi@huawei.com> 09/09/15 6:04 PM >>>
> Hi Zhen,
> 
> On 2015/9/9 13:16, Zhen Ren wrote:
>> 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
> I don't think 'cat' works here.
> Could you please use hexdump?
> 
>> 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@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
>>
>>
>>
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
>>
> 
> 
> 
> 
> 
> .
>
Ryan Ding Sept. 10, 2015, 1:53 a.m. UTC | #7
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@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);
>   	}
>
>   	/*
Andrew Morton Sept. 14, 2015, 10:22 p.m. UTC | #8
On Thu, 10 Sep 2015 09:53:11 +0800 ryding <ryan.ding@oracle.com> wrote:

> 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. ;-)

Yes, your "ocfs2: fill in the unused portion of the block with zeros by
dio_zero_block()" conflicts with this patch a little.

Does your patchset need alteration to fix this bug?
Ryan Ding Sept. 15, 2015, 2:34 a.m. UTC | #9
On 09/15/2015 06:22 AM, Andrew Morton wrote:
> On Thu, 10 Sep 2015 09:53:11 +0800 ryding <ryan.ding@oracle.com> wrote:
>
>> 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. ;-)
> Yes, your "ocfs2: fill in the unused portion of the block with zeros by
> dio_zero_block()" conflicts with this patch a little.
>
> Does your patchset need alteration to fix this bug?
My patchset "ocfs2: fix ocfs2 direct io code patch to support sparse 
file and
data ordering semantics" has already include the fix to this bug. 
Yiwen's patch
is no longer needed if my patchset is accepted.

Patch
diff mbox

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);
 	}

 	/*