xfs
[Top] [All Lists]

Re: [RFC PATCH 10/11] xfs: update the finobt on inode free

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [RFC PATCH 10/11] xfs: update the finobt on inode free
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 6 Sep 2013 10:28:47 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5228AF00.7080700@xxxxxxxxxx>
References: <1378232708-57156-1-git-send-email-bfoster@xxxxxxxxxx> <1378232708-57156-11-git-send-email-bfoster@xxxxxxxxxx> <20130905025421.GX23571@dastard> <5228AF00.7080700@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Sep 05, 2013 at 12:19:12PM -0400, Brian Foster wrote:
> On 09/04/2013 10:54 PM, Dave Chinner wrote:
> > On Tue, Sep 03, 2013 at 02:25:07PM -0400, Brian Foster wrote:
> >> An inode free operation can have several effects on the finobt. If
> >> all inodes have been freed and the chunk deallocated, we remove the
> >> finobt record. If the inode chunk was previously full, we must
> >> insert a new record based on the existing inobt record. Otherwise,
> >> we modify the record in place.
.....
> >> +  } else if ((i == 0) && (ibtrec->ir_freecount == 1)) {
> >> +          /*
> >> +           * No existing finobt record and the inobt record has a single
> >> +           * free inode. This means we've freed an inode in a previously
> >> +           * fully allocated chunk. Insert a new record into the finobt
> >> +           * based on the current inobt record.
> >> +           */
> >> +          cur->bc_rec.i.ir_startino = ibtrec->ir_startino;
> >> +          cur->bc_rec.i.ir_free = ibtrec->ir_free;
> >> +          cur->bc_rec.i.ir_freecount = ibtrec->ir_freecount;
> >> +          error = xfs_btree_insert(cur, &i);
> >> +          if (error)
> >> +                  goto error;
> >> +          ASSERT(i == 1);
> > 
> > That's rather similar to the code in xfs_inobt_insert(). Indeed,
> > is you write a helper - xfs_inobt_insert_rec() - for this, then rather than 
> > modifying
> > xfs_inobt_lookup() to take extra parameters like I wondered for the
> > previous patch, leave it alonge and pass the parameters to
> > xfs_inobt_insert_rec() instead.
> > 
> > Then this code is functionally identical to xfs_inobt_insert() done
> > during allocation....
> > 
> 
> I think I'm parsing you after having another look at the code.
> xfs_inobt_lookup() remains as is and is potentially used from
> xfs_inobt_insert(). xfs_inobt_insert_rec() is introduced to set the
> cursor fields and do the insert and is used here and from
> xfs_inobt_insert().

Effectively. xfs_inobt_insert() becomes:

        for (each inode chunk) {
                xfs_inobt_lookup(cur, startino)
                xfs_inobt_insert_rec(cur, startino, free, free_count)
        }

And this code becomes:

        xfs_inobt_lookup(cur, startino);
        if (!found) {
                if (free_count == 1)
                        xfs_inobt_insert_rec(cur, startino, free, free_count)
                else
                        CORRUPTION
                goto out;
        }

> At that point, this looks close to xfs_inobt_insert(), but I think using
> that here would introduce a duplicate lookup.

Yes, it would. I think just using helpers like this is sufficient
for the two different cases, especially as xfs_inobt_insert() needs
to be able to handle multiple chunk insertion and we don't have that
here...

> Regardless, we'll see what
> the whole thing looks like at that point. Thanks for the reviews. :)

No worries. BTW, can you post your rudimentary userspace support so
we can run tests that use this code, too?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>