xfs
[Top] [All Lists]

Re: [PATCH 08/18] xfs: create helper to manage record overlap for sparse

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 08/18] xfs: create helper to manage record overlap for sparse inode chunks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 29 Jul 2014 10:07:33 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140728161925.GD59542@xxxxxxxxxxxxxxx>
References: <1406211788-63206-1-git-send-email-bfoster@xxxxxxxxxx> <1406211788-63206-9-git-send-email-bfoster@xxxxxxxxxx> <20140724224112.GR20518@dastard> <20140728161925.GD59542@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Jul 28, 2014 at 12:19:25PM -0400, Brian Foster wrote:
> On Fri, Jul 25, 2014 at 08:41:12AM +1000, Dave Chinner wrote:
> > On Thu, Jul 24, 2014 at 10:22:58AM -0400, Brian Foster wrote:
> > > + struct xfs_btree_cur            *cur;
> > > + struct xfs_agi                  *agi = XFS_BUF_TO_AGI(agbp);
> > > + xfs_agnumber_t                  agno = be32_to_cpu(agi->agi_seqno);
> > > + xfs_agino_t                     previno;
> > > + int                             error;
> > > + int                             i;
> > > + struct xfs_inobt_rec_incore     rec;
> > > +
> > > + orec->ir_startino = NULLAGINO;
> > > +
> > > + cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);
> > > +
> > > + previno = newino + count - XFS_INODES_PER_CHUNK;
> > > + error = xfs_inobt_lookup(cur, previno, XFS_LOOKUP_GE, &i);
> > 
> > You want XFS_LOOKUP_EQ, yes? i.e. XFS_LOOKUP_GE won't fail if the
> > exact record for the inode chunk does not exist - it will return the
> > next one in the btree.
> > 
> 
> Assuming variable sparse chunk granularity,

Isn't the granularity fixed for the specific filesystem
configuration as part of the on-disk format?

> I don't really know the
> start ino of the record that potentially covers the new inode chunk.
> Given that, we use the smallest possible start ino that could include
> this chunk and search forward from there. As you've noted below, I
> wasn't relying on failure here to detect the scenario where there is no
> existing record.

Ok, that's not how I thought the code was attempting to implement
the "has record" check. My mistake - a comment explaining how the
match is supposed to work would be helpful, I think.

However, with that in mind, why do you even bother calculating at
"previno"?  If you want the chunk that the "newino" lies in, then
by definition it's going to be the first record at an equal or
lower start inode number than newino.  i.e.:

        xfs_inobt_lookup(cur, newino, XFS_LOOKUP_LE, &i);

Will return either:

        - a match with startino <= newino < startino + XFS_INODES_PER_CHUNK
        - a match with startino + XFS_INODES_PER_CHUNK <= newino
        - a failure due to no record.

i.e. the first case is the chunk record we want, the others are
"does not exist" failures. We don't need to calculate the "previno"
at all.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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