Re: Review: factor extracting extent size hints from the inode

[David Chinner <dgc@sgi.com>]

On Tue, Jun 12, 2007 at 03:13:26PM +1000, Vlad Apostolov wrote:
> David Chinner wrote:
> >Replace frequently repeated, open coded extraction of the
> >extent size hint from the xfs_inode with a single helper
> >function.
> >
> >Cheers,
> >
> >Dave.
> >  
> Dave,
> 
> I think XFS_DIFLAG_REALTIME and XFS_DIFLAG_EXTSIZE flags are
> mutually exclusive.

No, that's not true.

> XFS_DIFLAG_REALTIME and di_extsize have been introduced and used
> on Irix and Linux before XFS_DIFLAG_EXTSIZE.

Sure, but look at how we are supposed to set the extent size hint.
i.e XFS_IOC_FSSETXATTR, which by your own account should have the
XFS_XFLAG_EXTSIZE bit set in it. And that does not matter if
the file is realtime or not.

See the xfs_io code that sets the extent size hint:

    567         if (S_ISREG(stat.st_mode)) {
    568                 fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
    569         } else if (S_ISDIR(stat.st_mode)) {
    570                 fsx.fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
    571         } else {
    572                 printf(_("invalid target file type - file %s\n"), path);
    573                 return 0;
    574         }
    575         fsx.fsx_extsize = extsz;
    576
    577         if ((xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx)) < 0) {
    578                 printf("%s: XFS_IOC_FSSETXATTR %s: %s\n",
    579                         progname, path, strerror(errno));
    580                 return 0;
    581         }

If you use this method of setting the extent size hint, then you will
*always* get the XFS_DIFLAG_EXTSIZE flag set when you have an extent
size hint, regardless of whether it is a realtime file or not.

> This code:
> 
> +	if (unlikely(ip->i_d.di_flags & XFS_DIFLAG_REALTIME)) {
> +		extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)
> +				? ip->i_d.di_extsize
> +				: ip->i_mount->m_sb.sb_rextsize;
> +		ASSERT(extsz);
> +	} else {
> 
> shouldn't test for XFS_DIFLAG_EXTSIZE but use di_extsize if non zero.

Hmmmm - I think having one rule for realtime and a different rule
for data to do exactly the same thing is busted.

Like the realtime code, there are many places the non-realtime
code don't bother to check for a valid extent size hint flag - it
just read it blindly. That was part of the problem that the DMF
folks tripped over - the flag wasn't set but the hint was, and bad
things were happening because it wasn't consistently applied.

Also, if we want to fix up the setting of the extent size hint
so that it errors out if you don't set the XFS_XFLAG_EXTSIZE as
well, then we'd have to make an exception for the realtime inodes.

So, there's plenty of reasons for leaving this code as it stands
and have both realtime and non-realtime behave the same way.

Thoughts?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group