Re: [PATCH] ext4: don't consume reserved space when allocating partial cluster

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

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5416 bytes --]

On Mon, 23 Mar 2015, Eric Whitney wrote:

> Date: Mon, 23 Mar 2015 19:15:39 -0400
> From: Eric Whitney <enwlinux@gmail.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Eric Whitney <enwlinux@gmail.com>, linux-ext4@vger.kernel.org,
>     tytso@mit.edu
> Subject: Re: [PATCH] ext4: don't consume reserved space when allocating
>     partial cluster
> 
> * Lukáš Czerner <lczerner@redhat.com>:
> > On Mon, 16 Mar 2015, Eric Whitney wrote:
> > 
> > > Date: Mon, 16 Mar 2015 21:20:09 -0400
> > > From: Eric Whitney <enwlinux@gmail.com>
> > > To: linux-ext4@vger.kernel.org
> > > Cc: tytso@mit.edu
> > > Subject: [PATCH] ext4: don't consume reserved space when allocating partial
> > >     cluster
> > > 
> > > When xfstests' auto group is run on a bigalloc filesystem with a
> > > 4.0-rc3 kernel, e2fsck failures and kernel warnings occur for some
> > > tests. e2fsck reports incorrect iblocks values, and the warnings
> > > indicate that the space reserved by delayed allocation is being
> > > overdrawn at allocation time.
> > > 
> > > Some of these errors occur because the reserved space is incorrectly
> > > decreased by one cluster when ext4_ext_map_blocks satisfies an
> > > allocation request by using an unused portion of a previously allocated
> > > cluster.  Because a cluster's worth of reserved space was already
> > > removed when it was first allocated, it should not be removed again.
> > 
> > Hi Eric,
> > 
> > I am not sure I understand. What do you mean by saying that the
> > space was already removed when it was first allocated ?
> 
> Hi Lukas:
> 
> I'm sorry that was confusing - I didn't get the terminology quite right,
> given the usage in the code.  What I'm discussing in that sentence is
> the space reserved for delayed allocation.  Instead of "removed", I should
> have said "released".  If we're mapping from an existing cluster, at some
> point in the past that cluster was allocated, and at that time the space
> reservation for that cluster would have been released.  So, we ought not
> to be releasing its space again.
> 
> > 
> > From my point of view the ext4_da_update_reserve_space() call is ok,
> > because we're going to use blocks from already allocated cluster, so
> > we do not want to account for quota in this case, because that has
> > already been done when the cluster was allocated. The rest is just
> > updating reservations and the dirty clusters counter which needs to
> > be done in any case. But I might be actually missing something, am I
> > ?
> 
> I agree that we don't want to account for quota, as that should have been
> done in the past when the cluster was first allocated.  I think we don't
> want to update the reservations or the dirty clusters counter because that
> should also have been taken into account at the same time in the past.  If
> we update them again, decreasing them once more for the cluster we're currently
> processing, we'll be double accounting for the space.
> 
> The code in ext4_da_map_blocks() that runs at write begin time and increases
> the amount of reserved space only does so when a cluster has not been
> previously allocated or already accounted for as part of a delalloc extent
> recorded in the status tree.  I think it should be accurately reflecting the
> number of clusters we'll eventually need to allocate for data, so there's no
> room for double counting when mapping from an existing cluster in
> ext4_ext_map_blocks().

Ah, you're right so that's there probably from the times where we
had to do metadata blocks reservation as well. Thanks for
clarification.

> 
> If I'm not reading the delalloc accounting code incorrectly, a few more patches
> will likely be required to remove some of the code immediately following
> if (!map_from_cluster) and a chunk in ext4_ext_handle_unwritten_extents().

Maybe it would be worth deal with that mess in the
ext4_ext_map_blocks() within a single patch ?

-Lukas

> 
> Thanks,
> Eric
> 
> 
> > 
> > Thanks!
> > -Lukas
> > 
> > > 
> > > This patch appears to correct the e2fsck failure reported for
> > > generic/232 and the kernel warnings produced by ext4/001, generic/009,
> > > and generic/033.  Failures and warnings for some other tests remain to
> > > be addressed.
> > > 
> > > Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> > > ---
> > >  fs/ext4/extents.c | 14 +-------------
> > >  1 file changed, 1 insertion(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index bed4308..554190e 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -4535,19 +4535,7 @@ got_allocated_blocks:
> > >  		 */
> > >  		reserved_clusters = get_reserved_cluster_alloc(inode,
> > >  						map->m_lblk, allocated);
> > > -		if (map_from_cluster) {
> > > -			if (reserved_clusters) {
> > > -				/*
> > > -				 * We have clusters reserved for this range.
> > > -				 * But since we are not doing actual allocation
> > > -				 * and are simply using blocks from previously
> > > -				 * allocated cluster, we should release the
> > > -				 * reservation and not claim quota.
> > > -				 */
> > > -				ext4_da_update_reserve_space(inode,
> > > -						reserved_clusters, 0);
> > > -			}
> > > -		} else {
> > > +		if (!map_from_cluster) {
> > >  			BUG_ON(allocated_clusters < reserved_clusters);
> > >  			if (reserved_clusters < allocated_clusters) {
> > >  				struct ext4_inode_info *ei = EXT4_I(inode);
> > > 
>