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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
