Re: [PATCH] Introduce SEEK_DATA/SEEK_HOLE to XFS V2

[Christoph Hellwig <hch@infradead.org>]

On Thu, Nov 24, 2011 at 02:23:31PM +1100, Dave Chinner wrote:
> > > +		if (map[0].br_startblock == HOLESTARTBLOCK) {
> > > +			if (map[1].br_startblock == HOLESTARTBLOCK) {
> > > +				fsbno = map[1].br_startoff +
> > > +					map[1].br_blockcount;
> > 
> > I don't think this code is reachable - xfs_bmapi will never produce
> > multiple consecutive HOLESTARTBLOCK extents.
> 
> If the hole would overflow br_blockcount (32 bits of FSB units, 16TB
> by default), then we should get multiple consecutive hole records
> returned.

Right, the XFS_FILBLKS_MIN in xfs_bmapi_read will limit it, and we'll
it the same case again in the loop.  So yes, we'll need it; and we
should have a test to verify this case.

> > This also means that we never have to loop here until we add dirty
> > unwritten probing - if the second extent doesn't contain data there
> > won't be any other data extent in this file beyound our offset.
> 
> I think that's incorrect. A data extent is limited in length by the
> on disk format (21 bits of FSB, 8GB in 4k block fs), so if you've
> got more than 8GB of data or the file is fragmented after the
> current extent then we can still get back multiple data extents
> before we find the next hole.

Indeed.  Add fragmented file to what we need to test in QA..

> > 
> > I think just checking for br_state == XFS_EXT_NORM should be fine here,
> > as unwritten extents can be delayed allocated.
> 
> Can they? I'm pretty sure delalloc and unwritten are mutually
> exclusive states for an extent.

Yes, they _can't_.  That was a typo, the rest of the setentence wouldn't
make sense if that was allowed.

> > > +	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
> > > +	    ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
> > > +	    ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> > > +		return XFS_ERROR(EINVAL);
> > 
> > I'd recommend moving this check into xfs_file_llseek and even do it
> > for the normal lseek requests - it's another sanity check for corrupted
> > filesystems which makes sense everywhere.  I also think the return value
> > should be EFSCORRUPTED.
> >
> > Also XFS_DINODE_FMT_LOCAL isn't valid for regular files (yet) so it
> > shouldn't be tested for.
> 
> I don't think it is necessary at all - the low level bmap functions
> already do these checks.

Indeed, although xfs_bmap_first_unused also allows XFS_DINODE_FMT_LOCAL
format, but I think that is fine.

> > > +	lock = xfs_ilock_map_shared(ip);
> > > +
> > > +	if (XFS_FORCED_SHUTDOWN(mp)) {
> > > +		error = EIO;
> > > +		goto out_lock;
> > > +	}
> > 
> > The shutdown check probably should go into the common lseek code and
> > done for all cases.
> 
> I think the low level functions also do this check so it's not
> explicitly needed here, anyway.

xfs_bmapi_read does it, xfs_bmap_first_unused lacks it.  And returning
an error ASAP on a normal lseek for the normal lseek cases also makes
a lot of sense.

> > 
> > > +
> > > +	XFS_STATS_INC(xs_blk_mapr);
> > 
> > I don't think this counter should be incremented here.
> 
> It's done in the lower layer functions, so shouldn't be here.

It is for xfs_bmapi_read, it isn't for xfs_bmap_first_unused, and then
again it really shouldn't either - it's a counter for xfs_bmapi_read
calls.

> > Now that just the locking and the xfs_iread_extents call is left in
> > this function I'd suggest to remove it and instead add duplicates
> > of the locking and xfs_iread_extents into xfs_seek_hole and
> > xfs_seek_data.
> 
> Actually, it just turns into "lock, call seek/data fucntion, unlock",
> so it can probaly go away entirely.

That's what I tried to imply with the above comment.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs