On Mon, 8 Jun 2015, Lukáš Czerner wrote:
> Date: Mon, 8 Jun 2015 17:45:08 +0200 (CEST)
> From: Lukáš Czerner <lczerner@xxxxxxxxxx>
> To: Theodore Ts'o <tytso@xxxxxxx>
> Cc: linux-ext4@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] ext4: fix reservation release on invalidatepage for
> delalloc fs
>
> 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
Hi Ted,
can you take a second look at the 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
>
