[PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Yongqiang Yang]

If there is a unwritten but clean buffer in a page and there is a dirty buffer
after the buffer, then mpage_submit_io does not write the dirty buffer out.
As a result, da_writepages loops forever.

This patch fixes the problem by checking dirty flag.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/inode.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 755f6c7..20a1d17 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct mpage_da_data *mpd,
 					clear_buffer_unwritten(bh);
 				}
 
-				/* skip page if block allocation undone */
-				if (buffer_delay(bh) || buffer_unwritten(bh))
+				/*
+				 * skip page if block allocation undone and
+				 * block is dirty
+				 */
+				if (ext4_bh_delay_or_unwritten(NULL, bh))
 					skip_page = 1;
 				bh = bh->b_this_page;
 				block_start += bh->b_size;
-- 
1.7.5.1

[PATCH 2/2] ext4: let ext4_discard_partial_buffers handle pages without buffers correctly

[Yongqiang Yang]

If a page has been read to memory and never been written, it has no buffers,
but we should handle the page in truncate or punch hole.

Vfs code of writing operations has handled holes correctly, so this patch removes
the code handling hole in writing operations.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/inode.c |   41 ++---------------------------------------
 1 files changed, 2 insertions(+), 39 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 20a1d17..bd793d6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2385,7 +2385,6 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
 	pgoff_t index;
 	struct inode *inode = mapping->host;
 	handle_t *handle;
-	loff_t page_len;
 
 	index = pos >> PAGE_CACHE_SHIFT;
 
@@ -2432,13 +2431,6 @@ retry:
 		 */
 		if (pos + len > inode->i_size)
 			ext4_truncate_failed_write(inode);
-	} else {
-		page_len = pos & (PAGE_CACHE_SIZE - 1);
-		if (page_len > 0) {
-			ret = ext4_discard_partial_page_buffers_no_lock(handle,
-				inode, page, pos - page_len, page_len,
-				EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED);
-		}
 	}
 
 	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
@@ -2481,7 +2473,6 @@ static int ext4_da_write_end(struct file *file,
 	loff_t new_i_size;
 	unsigned long start, end;
 	int write_mode = (int)(unsigned long)fsdata;
-	loff_t page_len;
 
 	if (write_mode == FALL_BACK_TO_NONDELALLOC) {
 		if (ext4_should_order_data(inode)) {
@@ -2530,16 +2521,6 @@ static int ext4_da_write_end(struct file *file,
 	}
 	ret2 = generic_write_end(file, mapping, pos, len, copied,
 							page, fsdata);
-
-	page_len = PAGE_CACHE_SIZE -
-			((pos + copied - 1) & (PAGE_CACHE_SIZE - 1));
-
-	if (page_len > 0) {
-		ret = ext4_discard_partial_page_buffers_no_lock(handle,
-			inode, page, pos + copied - 1, page_len,
-			EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED);
-	}
-
 	copied = ret2;
 	if (ret2 < 0)
 		ret = ret2;
@@ -3201,26 +3182,8 @@ int ext4_discard_partial_page_buffers_no_lock(handle_t *handle,
 
 	iblock = index << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits);
 
-	if (!page_has_buffers(page)) {
-		/*
-		 * If the range to be discarded covers a partial block
-		 * we need to get the page buffers.  This is because
-		 * partial blocks cannot be released and the page needs
-		 * to be updated with the contents of the block before
-		 * we write the zeros on top of it.
-		 */
-		if ((from & (blocksize - 1)) ||
-		    ((from + length) & (blocksize - 1))) {
-			create_empty_buffers(page, blocksize, 0);
-		} else {
-			/*
-			 * If there are no partial blocks,
-			 * there is nothing to update,
-			 * so we can return now
-			 */
-			return 0;
-		}
-	}
+	if (!page_has_buffers(page))
+		create_empty_buffers(page, blocksize, 0);
 
 	/* Find the buffer that contains "offset" */
 	bh = page_buffers(page);
-- 
1.7.5.1

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Tao Ma]

Hi Yongqiang,
On 11/23/2011 05:15 PM, Yongqiang Yang wrote:
> If there is a unwritten but clean buffer in a page and there is a dirty buffer
> after the buffer, then mpage_submit_io does not write the dirty buffer out.
> As a result, da_writepages loops forever.
Did you ever meet with this bug or just find it to be a possible bug by
skimming the code? Actually, I can't find a proper way to get a buffer
which can satisfy the check.

Thanks
Tao
> 
> This patch fixes the problem by checking dirty flag.
> 
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
>  fs/ext4/inode.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 755f6c7..20a1d17 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct mpage_da_data *mpd,
>  					clear_buffer_unwritten(bh);
>  				}
>  
> -				/* skip page if block allocation undone */
> -				if (buffer_delay(bh) || buffer_unwritten(bh))
> +				/*
> +				 * skip page if block allocation undone and
> +				 * block is dirty
> +				 */
> +				if (ext4_bh_delay_or_unwritten(NULL, bh))
>  					skip_page = 1;
>  				bh = bh->b_this_page;
>  				block_start += bh->b_size;

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Tao Ma]

On 11/26/2011 11:13 PM, Yongqiang Yang wrote:
> Hi,
> 
> It can be tested by xfstests 61 or 91, I did not remember which one.
ok, I will try it later. But I still wonder how it happens. See my
comments below.
> 
> It can be reproduced by reading on a fallocted block and write the block
> after the fallocted block.  Then the written block can not be written
> out by da_writepages.
Why? in read no bh will be created, and in write only the written bh
will be mapped and set unwritten. How could that happen? Sorry, but this
explanation doesn't convince me.
> 
> Yongqiang.
> 
> On Saturday, November 26, 2011, Tao Ma <tm@tao.ma <mailto:tm@tao.ma>> wrote:
>> Hi Yongqiang,
>> On 11/23/2011 05:15 PM, Yongqiang Yang wrote:
>>> If there is a unwritten but clean buffer in a page and there is a
> dirty buffer
>>> after the buffer, then mpage_submit_io does not write the dirty
> buffer out.
>>> As a result, da_writepages loops forever.
>> Did you ever meet with this bug or just find it to be a possible bug by
>> skimming the code? Actually, I can't find a proper way to get a buffer
>> which can satisfy the check.
>>
>> Thanks
>> Tao
>>>
>>> This patch fixes the problem by checking dirty flag.
>>>
>>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com
> <mailto:xiaoqiangnk@gmail.com>>
>>> ---
>>>  fs/ext4/inode.c |    7 +++++--
>>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>> index 755f6c7..20a1d17 100644
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct
> mpage_da_data *mpd,
>>>                                       clear_buffer_unwritten(bh);
>>>                               }
>>>
>>> -                             /* skip page if block allocation undone */
>>> -                             if (buffer_delay(bh) ||
> buffer_unwritten(bh))
>>> +                             /*
>>> +                              * skip page if block allocation undone and
>>> +                              * block is dirty
>>> +                              */
>>> +                             if (ext4_bh_delay_or_unwritten(NULL, bh))
>>>                                       skip_page = 1;
>>>                               bh = bh->b_this_page;
>>>                               block_start += bh->b_size;
>>
>>
>>
> 
> -- 
> Best Wishes
> Yongqiang Yang
> 

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Tao Ma]

On 11/27/2011 09:29 AM, Yongqiang Yang wrote:
> 
> 
> On Saturday, November 26, 2011, Tao Ma <tm@tao.ma <mailto:tm@tao.ma>> wrote:
>> On 11/26/2011 11:13 PM, Yongqiang Yang wrote:
>>> Hi,
>>>
>>> It can be tested by xfstests 61 or 91, I did not remember which one.
>> ok, I will try it later. But I still wonder how it happens. See my
>> comments below.
>>>
>>> It can be reproduced by reading on a fallocted block and write the block
>>> after the fallocted block.  Then the written block can not be written
>>> out by da_writepages.
>> Why? in read no bh will be created, and in write only the written bh
>> will be mapped and set unwritten. How could that happen? Sorry, but this
>> explanation doesn't convince me.
> Ok!  The bug was found with punch hole enabled, and punch hole creates
> empty buffers sometimes.
> Actually, punching hole can read the whole page like read operation
> does, but it do not.  May be this is the source of the  problem.
> The problemastic  function is discard_partial_buffers.   I post a patch
> fixing another bug in it in this series.  If we read whole page in this
> function, the problem can also be solved.
Got it and thanks for the explanation. So we introduced a bug and then
fix it in another way. ;)

Thanks
Tao
> 
> Yongqiang.
>>>
>>> Yongqiang.
>>>
>>> On Saturday, November 26, 2011, Tao Ma <tm@tao.ma <mailto:tm@tao.ma>
> <mailto:tm@tao.ma <mailto:tm@tao.ma>>> wrote:
>>>> Hi Yongqiang,
>>>> On 11/23/2011 05:15 PM, Yongqiang Yang wrote:
>>>>> If there is a unwritten but clean buffer in a page and there is a
>>> dirty buffer
>>>>> after the buffer, then mpage_submit_io does not write the dirty
>>> buffer out.
>>>>> As a result, da_writepages loops forever.
>>>> Did you ever meet with this bug or just find it to be a possible bug by
>>>> skimming the code? Actually, I can't find a proper way to get a buffer
>>>> which can satisfy the check.
>>>>
>>>> Thanks
>>>> Tao
>>>>>
>>>>> This patch fixes the problem by checking dirty flag.
>>>>>
>>>>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com
> <mailto:xiaoqiangnk@gmail.com>
>>> <mailto:xiaoqiangnk@gmail.com <mailto:xiaoqiangnk@gmail.com>>>
>>>>> ---
>>>>>  fs/ext4/inode.c |    7 +++++--
>>>>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>>>> index 755f6c7..20a1d17 100644
>>>>> --- a/fs/ext4/inode.c
>>>>> +++ b/fs/ext4/inode.c
>>>>> @@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct
>>> mpage_da_data *mpd,
>>>>>                                       clear_buffer_unwritten(bh);
>>>>>                               }
>>>>>
>>>>> -                             /* skip page if block allocation
> undone */
>>>>> -                             if (buffer_delay(bh) ||
>>> buffer_unwritten(bh))
>>>>> +                             /*
>>>>> +                              * skip page if block allocation
> undone and
>>>>> +                              * block is dirty
>>>>> +                              */
>>>>> +                             if (ext4_bh_delay_or_unwritten(NULL, bh))
>>>>>                                       skip_page = 1;
>>>>>                               bh = bh->b_this_page;
>>>>>                               block_start += bh->b_size;
>>>>
>>>>
>>>>
>>>
>>> --
>>> Best Wishes
>>> Yongqiang Yang
>>>
>>
>>
> 
> -- 
> Best Wishes
> Yongqiang Yang
> 

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Allison Henderson]

On 11/23/2011 02:15 AM, Yongqiang Yang wrote:
> If there is a unwritten but clean buffer in a page and there is a dirty buffer
> after the buffer, then mpage_submit_io does not write the dirty buffer out.
> As a result, da_writepages loops forever.
>
> This patch fixes the problem by checking dirty flag.
>
> Signed-off-by: Yongqiang Yang<xiaoqiangnk@gmail.com>
> ---
>   fs/ext4/inode.c |    7 +++++--
>   1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 755f6c7..20a1d17 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct mpage_da_data *mpd,
>   					clear_buffer_unwritten(bh);
>   				}
>
> -				/* skip page if block allocation undone */
> -				if (buffer_delay(bh) || buffer_unwritten(bh))
> +				/*
> +				 * skip page if block allocation undone and
> +				 * block is dirty
> +				 */
> +				if (ext4_bh_delay_or_unwritten(NULL, bh))
>   					skip_page = 1;
>   				bh = bh->b_this_page;
>   				block_start += bh->b_size;

Hi Yongqiang,

Thank you for looking into the punch hole code, I know there's been some 
recent bugs reported, so I am looking at it too.  I've applied your 
patch and ran it through an fsx stress test, and I notice there are some 
failures, but it appears to run longer with the patch then with out it, 
so it may not be the cause of the errors I'm seeing. I think maybe 
something else may have happened between now and the last time it made 
it through 24hr of fsx (at least for me :) ), so I'm continuing to look 
through the recent code changes.  I will keep folks posted on my 
findings.  Thx!

Allison Henderson

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Yongqiang Yang]

On Fri, Dec 2, 2011 at 4:13 AM, Allison Henderson
<achender@linux.vnet.ibm.com> wrote:
> On 11/23/2011 02:15 AM, Yongqiang Yang wrote:
>>
>> If there is a unwritten but clean buffer in a page and there is a dirty
>> buffer
>> after the buffer, then mpage_submit_io does not write the dirty buffer
>> out.
>> As a result, da_writepages loops forever.
>>
>> This patch fixes the problem by checking dirty flag.
>>
>> Signed-off-by: Yongqiang Yang<xiaoqiangnk@gmail.com>
>> ---
>>  fs/ext4/inode.c |    7 +++++--
>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 755f6c7..20a1d17 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct mpage_da_data
>> *mpd,
>>                                        clear_buffer_unwritten(bh);
>>                                }
>>
>> -                               /* skip page if block allocation undone */
>> -                               if (buffer_delay(bh) ||
>> buffer_unwritten(bh))
>> +                               /*
>> +                                * skip page if block allocation undone
>> and
>> +                                * block is dirty
>> +                                */
>> +                               if (ext4_bh_delay_or_unwritten(NULL, bh))
>>                                        skip_page = 1;
>>                                bh = bh->b_this_page;
>>                                block_start += bh->b_size;
>
> Hi Yongqiang,
>
> Thank you for looking into the punch hole code, I know there's been some
> recent bugs reported, so I am looking at it too.  I've applied your patch
> and ran it through an fsx stress test, and I notice there are some failures,
> but it appears to run longer with the patch then with out it, so it may not
> be the cause of the errors I'm seeing. I think maybe something else may have
> happened between now and the last time it made it through 24hr of fsx (at
> least for me :) ), so I'm continuing to look through the recent code
On the other hand, xfstests have a lot of changes since your last
test.   I am not sure if original xfstests did not discover some
errors.
> changes.  I will keep folks posted on my findings.  Thx!
Did you test it by multi-thread tests or single thread tests?  If
multi-thread, I suggest that we hold the i_mutex in punching hole.


Yongqiang.
>
> Allison Henderson
>
>



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Allison Henderson]

On 12/01/2011 06:15 PM, Yongqiang Yang wrote:
> On Fri, Dec 2, 2011 at 4:13 AM, Allison Henderson
> <achender@linux.vnet.ibm.com>  wrote:
>> On 11/23/2011 02:15 AM, Yongqiang Yang wrote:
>>>
>>> If there is a unwritten but clean buffer in a page and there is a dirty
>>> buffer
>>> after the buffer, then mpage_submit_io does not write the dirty buffer
>>> out.
>>> As a result, da_writepages loops forever.
>>>
>>> This patch fixes the problem by checking dirty flag.
>>>
>>> Signed-off-by: Yongqiang Yang<xiaoqiangnk@gmail.com>
>>> ---
>>>   fs/ext4/inode.c |    7 +++++--
>>>   1 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>> index 755f6c7..20a1d17 100644
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -1339,8 +1339,11 @@ static int mpage_da_submit_io(struct mpage_da_data
>>> *mpd,
>>>                                         clear_buffer_unwritten(bh);
>>>                                 }
>>>
>>> -                               /* skip page if block allocation undone */
>>> -                               if (buffer_delay(bh) ||
>>> buffer_unwritten(bh))
>>> +                               /*
>>> +                                * skip page if block allocation undone
>>> and
>>> +                                * block is dirty
>>> +                                */
>>> +                               if (ext4_bh_delay_or_unwritten(NULL, bh))
>>>                                         skip_page = 1;
>>>                                 bh = bh->b_this_page;
>>>                                 block_start += bh->b_size;
>>
>> Hi Yongqiang,
>>
>> Thank you for looking into the punch hole code, I know there's been some
>> recent bugs reported, so I am looking at it too.  I've applied your patch
>> and ran it through an fsx stress test, and I notice there are some failures,
>> but it appears to run longer with the patch then with out it, so it may not
>> be the cause of the errors I'm seeing. I think maybe something else may have
>> happened between now and the last time it made it through 24hr of fsx (at
>> least for me :) ), so I'm continuing to look through the recent code
> On the other hand, xfstests have a lot of changes since your last
> test.   I am not sure if original xfstests did not discover some
> errors.
>> changes.  I will keep folks posted on my findings.  Thx!
> Did you test it by multi-thread tests or single thread tests?  If
> multi-thread, I suggest that we hold the i_mutex in punching hole.

Alrighty, well the test Im using is just the fsx test (in xfstests under 
the ltp folder).  I will check and see if there's been any updates to it 
since then.  The command I usually use is just "./fsx -d -S 1 
/mnt/ext4MntPt/test" from the ltp folder.  I dont think it's 
multi-threaded, but the i_mutex lock is another work item on my plate 
that I haven't gotten to yet.  The reason we dont want to just lock 
i_mutex is because folks are trying to reduce the use of i_mutex in 
ext4.  So the plan is to implement extent locks to replace i_mutex all 
together.  That's another project, but I will do a trial run with 
i_mutex locked just to rule it out.

Allison Henderson

>
>
> Yongqiang.
>>
>> Allison Henderson
>>
>>
>
>
>

Re: [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize

[Ted Ts'o]

On Wed, Nov 23, 2011 at 05:15:25PM +0800, Yongqiang Yang wrote:
> If there is a unwritten but clean buffer in a page and there is a dirty buffer
> after the buffer, then mpage_submit_io does not write the dirty buffer out.
> As a result, da_writepages loops forever.
> 
> This patch fixes the problem by checking dirty flag.
> 
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>

Thanks, applied.

						- Ted

Re: [PATCH 2/2] ext4: let ext4_discard_partial_buffers handle pages without buffers correctly

[Ted Ts'o]

On Wed, Nov 23, 2011 at 05:15:26PM +0800, Yongqiang Yang wrote:
> If a page has been read to memory and never been written, it has no buffers,
> but we should handle the page in truncate or punch hole.
> 
> Vfs code of writing operations has handled holes correctly, so this patch removes
> the code handling hole in writing operations.
> 
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>

Thanks, applied.

						- Ted