xfs
[Top] [All Lists]

Re: [PATCH 08/25] xfs: introduce xfs_bmapi_delay()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 08/25] xfs: introduce xfs_bmapi_delay()
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 7 Sep 2011 12:21:20 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110906152706.GA14728@xxxxxxxxxxxxx>
References: <20110824060428.789245205@xxxxxxxxxxxxxxxxxxxxxx> <20110824060641.979845427@xxxxxxxxxxxxxxxxxxxxxx> <1315002238.2069.88.camel@doink> <20110906152706.GA14728@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Tue, 2011-09-06 at 11:27 -0400, Christoph Hellwig wrote:
> > > + xfs_extnum_t            lastx;  /* last useful extent number */
> > > + int                     eof;    /* we've hit the end of extents */
> > > + int                     n = 0;  /* current extent index */
> > > + int                     error = 0;
> > > +
> > > + ASSERT(*nmap >= 1);
> > > + ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
> > > + ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
> > > +
> > 
> > Rearrange the following test to use the pattern (assigning error)
> > used in xfs_bmapi_read().
> 
> Hmm - given that error is used as a boolean there I don't actually like
> that pattern very much as error is generally used to hold an errno
> value.

My main thought was "make them consistent."  But
looking at it again I agree that "error" is not
the right name.  It doesn't really need changing
but could be beautified in some later patch.

> > 
> > > + if (unlikely(XFS_TEST_ERROR(
> > > +     (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> > > +      XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> > 
> > Are you certain that XFS_DINODE_FMT_LOCAL is not possible here?
> > I tried to trace it back but I'm still not sure.  The transaction
> > pointer passed is null, so it would have tripped an assertion
> > in the previous code.  (A simple explanation would reassure me.)
> 
> We can't hit it because we do not support the local format for regular
> files at all, and we do not support delayed allocations for anything
> but regular files.

OK.  That's another thing I didn't realize about XFS...

                                        -Alex


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