Re: [PATCH v2 2/2] mkfs: pass a custom cowextsize into the created filesystem

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Sep 08, 2017 at 01:02:15PM -0500, Eric Sandeen wrote:
> On 9/4/17 12:45 PM, Darrick J. Wong wrote:
> > Create a -d option to mkfs.xfs that enables administrators to set
> > the CoW extent size hint on the created files.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: port flags2diflags from the kernel
> 
> hm, I wonder if we need a libxfs/util.c in the kernel for stuff like
> this so libxfs diff will keep such things in sync in the future.
> 
> a project for another day I guess.  One question below.
> 
> > ---
> >  libxfs/util.c       |   64 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >  man/man8/mkfs.xfs.8 |    7 ++++++
> >  mkfs/xfs_mkfs.c     |   20 ++++++++++++++++
> >  3 files changed, 89 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libxfs/util.c b/libxfs/util.c
> > index 0e2f29e..b600594 100644
> > --- a/libxfs/util.c
> > +++ b/libxfs/util.c
> > @@ -175,6 +175,64 @@ libxfs_trans_ichgtime(
> >  	}
> >  }
> >  
> > +STATIC uint16_t
> > +xfs_flags2diflags(
> > +	struct xfs_inode	*ip,
> > +	unsigned int		xflags)
> > +{
> > +	/* can't set PREALLOC this way, just preserve it */
> > +	uint16_t		di_flags =
> > +		(ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> > +
> > +	if (xflags & FS_XFLAG_IMMUTABLE)
> > +		di_flags |= XFS_DIFLAG_IMMUTABLE;
> > +	if (xflags & FS_XFLAG_APPEND)
> > +		di_flags |= XFS_DIFLAG_APPEND;
> > +	if (xflags & FS_XFLAG_SYNC)
> > +		di_flags |= XFS_DIFLAG_SYNC;
> > +	if (xflags & FS_XFLAG_NOATIME)
> > +		di_flags |= XFS_DIFLAG_NOATIME;
> > +	if (xflags & FS_XFLAG_NODUMP)
> > +		di_flags |= XFS_DIFLAG_NODUMP;
> > +	if (xflags & FS_XFLAG_NODEFRAG)
> > +		di_flags |= XFS_DIFLAG_NODEFRAG;
> > +	if (xflags & FS_XFLAG_FILESTREAM)
> > +		di_flags |= XFS_DIFLAG_FILESTREAM;
> > +	if (S_ISDIR(VFS_I(ip)->i_mode)) {
> > +		if (xflags & FS_XFLAG_RTINHERIT)
> > +			di_flags |= XFS_DIFLAG_RTINHERIT;
> > +		if (xflags & FS_XFLAG_NOSYMLINKS)
> > +			di_flags |= XFS_DIFLAG_NOSYMLINKS;
> > +		if (xflags & FS_XFLAG_EXTSZINHERIT)
> > +			di_flags |= XFS_DIFLAG_EXTSZINHERIT;
> > +		if (xflags & FS_XFLAG_PROJINHERIT)
> > +			di_flags |= XFS_DIFLAG_PROJINHERIT;
> > +	} else if (S_ISREG(VFS_I(ip)->i_mode)) {
> > +		if (xflags & FS_XFLAG_REALTIME)
> > +			di_flags |= XFS_DIFLAG_REALTIME;
> > +		if (xflags & FS_XFLAG_EXTSIZE)
> > +			di_flags |= XFS_DIFLAG_EXTSIZE;
> > +	}
> > +
> > +	return di_flags;
> > +}
> > +
> > +STATIC uint64_t
> > +xfs_flags2diflags2(
> > +	struct xfs_inode	*ip,
> > +	unsigned int		xflags)
> > +{
> > +	uint64_t		di_flags2 =
> > +		(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
> > +
> > +	if (xflags & FS_XFLAG_DAX)
> > +		di_flags2 |= XFS_DIFLAG2_DAX;
> > +	if (xflags & FS_XFLAG_COWEXTSIZE)
> > +		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> > +
> > +	return di_flags2;
> > +}
> > +
> >  /*
> >   * Allocate an inode on disk and return a copy of its in-core version.
> >   * Set mode, nlink, and rdev appropriately within the inode.
> > @@ -254,15 +312,17 @@ libxfs_ialloc(
> >  	ip->i_d.di_extsize = pip ? 0 : fsx->fsx_extsize;
> >  	ip->i_d.di_dmevmask = 0;
> >  	ip->i_d.di_dmstate = 0;
> > -	ip->i_d.di_flags = pip ? 0 : fsx->fsx_xflags;
> > +	ip->i_d.di_flags = pip ? 0 : xfs_flags2diflags(ip, fsx->fsx_xflags);
> 
> is this a bugfix?

No.

Prior to this patch, the only fsx_xflags bits that mkfs could set are
the ones that correspond exactly to di_flags bits, so it was fine to set
them directly.  Subtle and annoying, but it worked.

However, the xfs_mkfs.c changes enable us to set FS_XFLAG_COWEXTSIZE,
which doesn't correspond to a di_flags bit, so now we need translation
functions to return the correct di_flags/di_flags2 values for the given
fsx_xflags.

(Granted you could argue that COWEXTSIZE is bit 17 so it'll just get
truncated when we set di_flags and therefore it's not absolutely
necessary to have a translation function for di_flags but that's
sloppy.)

--D

> The rest seems ok, just wondering if the above should be hidden in here -
> at least a mention of the change in the changelog seems needed, but most
> likely thjis is a separate change, no?  Or am I missing something?
> 
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html