Re: [PATCH] ext4: fix reservation release on invalidatepage for delalloc fs

["Lukáš Czerner" <lczerner@redhat.com>]

On Mon, 8 Jun 2015, Theodore Ts'o wrote:

> Date: Mon, 8 Jun 2015 11:16:51 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH] ext4: fix reservation release on invalidatepage for
>     delalloc fs
> 
> On Thu, Jun 04, 2015 at 10:25:01AM +0200, Lukas Czerner wrote:
> > On delalloc enabled file system on invalidatepage operation
> > in ext4_da_page_release_reservation() we want to clear the delayed
> > buffer and remove the extent covering the delayed buffer from the extent
> > status tree.
> > 
> > However currently there is a bug where on the systems with page size >
> > block size we will always remove extents from the start of the page
> > regardless where the actual delayed buffers are positioned in the page.
> 
> Right, because we end up screwing up the accounting.
> 
> > @@ -1363,14 +1363,23 @@ static void ext4_da_page_release_reservation(struct page *page,
> >  
> >  		if ((offset <= curr_off) && (buffer_delay(bh))) {
> >  			to_release++;
> > +			contiguous_blks++;
> >  			clear_buffer_delay(bh);
> > +		} else if (contiguous_blks) {
> > +			lblk = page->index <<
> > +			       (PAGE_CACHE_SHIFT - inode->i_blkbits);
> > +			lblk += (curr_off >> inode->i_blkbits) -
> > +				contiguous_blks;
> > +			ext4_es_remove_extent(inode, lblk, contiguous_blks);
> > +			contiguous_blks = 0;
> >  		}
> >  		curr_off = next_off;
> >  	} while ((bh = bh->b_this_page) != head);
> 
> Shouldn't we call ext4_es_remove_extent() on the portion of the page
> containing the delayed allocation region, before we clear
> contiguous_blks and resetting lblk?

Hi Ted,

right this is the point of the patch.

> 
> For example, suppose we had the 4k page with a 1k block size, where
> the first, second, and fourth blocks are delayed allocated.  With this
> patch we will end up only clearing the extent status tree for the
> fourth block, but not the first and second.

So when the first and second block are delayed, then the

if ((offset <= curr_off) && (buffer_delay(bh)))

will hit twice which means that we'll have contiguous_blks = 2

Now on the third block this condition will no longer be true
(because buffer_delay(bh) will be false) and so we will hit

else if (contiguous_blks) {

then lblk will be: start of the page + (curr_off - contiguous_blks).
curr_off at this point will point at third block (index 2) and
contiguous_blks is 2. Which means that lblk will point at the start
of the page - which is exactly right because the first delayed block
is at the start of the page.

So ext4_es_remove_extent() will remove extent of two blocks starting
from the end of the page - which means it removes first and second
delayed block.

Now when we check fourth block the

if ((offset <= curr_off) && (buffer_delay(bh)))

will hit again, leaving contiguous_blks with 1, then we leave the
while cycle and hit this:

if (contiguous_blks)

removing the extent starting at fourth block in the page removing
one block (the fourth block in the page).

That's how I wrote the code, but maybe I am missing something ? I am
a bit tired today already so my explanation is not very good, sorry.

Can you put your question in a form of a patch ?

Thanks!
-Lukas

> 
>        	      	      	  	    - Ted
>