On Mon, 8 Jun 2015, Theodore Ts'o wrote:
> Date: Mon, 8 Jun 2015 11:16:51 -0400
> From: Theodore Ts'o <tytso@xxxxxxx>
> To: Lukas Czerner <lczerner@xxxxxxxxxx>
> Cc: linux-ext4@xxxxxxxxxxxxxxx
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
