Re: [PATCH v13 14/14] xfsprogs: Add delayed attribute flag to cmd

From: Allison Henderson <allison.henderson@oracle.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v13 14/14] xfsprogs: Add delayed attribute flag to cmd
Date: Thu, 12 Nov 2020 20:52:57 -0700	[thread overview]
Message-ID: <6c04237c-e8d1-5b53-329d-735a8407b555@oracle.com> (raw)
In-Reply-To: <20201110234834.GM9695@magnolia>

On 11/10/20 4:48 PM, Darrick J. Wong wrote:
> On Thu, Oct 22, 2020 at 11:33:06PM -0700, Allison Henderson wrote:
>> From: Allison Collins <allison.henderson@oracle.com>
>>
>> mkfs: enable feature bit in mkfs via the '-n delattr' parameter.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> 
> I think it's sufficient to have one signoff here.
Sorry, still cleaning those out

> 
>> ---
>>   mkfs/xfs_mkfs.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 8fe149d..e18fb3a 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -94,6 +94,7 @@ enum {
>>   	N_SIZE = 0,
>>   	N_VERSION,
>>   	N_FTYPE,
>> +	N_DELATTR,
>>   	N_MAX_OPTS,
>>   };
>>   
>> @@ -547,6 +548,7 @@ static struct opt_params nopts = {
>>   		[N_SIZE] = "size",
>>   		[N_VERSION] = "version",
>>   		[N_FTYPE] = "ftype",
>> +		[N_DELATTR] = "delattr",
>>   	},
>>   	.subopt_params = {
>>   		{ .index = N_SIZE,
>> @@ -569,6 +571,12 @@ static struct opt_params nopts = {
>>   		  .maxval = 1,
>>   		  .defaultval = 1,
>>   		},
>> +		{ .index = N_DELATTR,
>> +		  .conflicts = { { NULL, LAST_CONFLICT } },
>> +		  .minval = 0,
>> +		  .maxval = 1,
>> +		  .defaultval = 1,
>> +		},
>>   	},
>>   };
>>   
>> @@ -742,6 +750,7 @@ struct sb_feat_args {
>>   	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
>>   	bool	nodalign;
>>   	bool	nortalign;
>> +	bool	delattr;		/* XFS_SB_FEAT_INCOMPAT_LOG_DELATTR */
>>   };
>>   
>>   struct cli_params {
>> @@ -873,7 +882,7 @@ usage( void )
>>   /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
>>   			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\
>>   /* label */		[-L label (maximum 12 characters)]\n\
>> -/* naming */		[-n size=num,version=2|ci,ftype=0|1]\n\
>> +/* naming */		[-n size=num,version=2|ci,ftype=0|1,delattr=0|1]\n\
>>   /* no-op info only */	[-N]\n\
>>   /* prototype file */	[-p fname]\n\
>>   /* quiet */		[-q]\n\
>> @@ -1592,6 +1601,9 @@ naming_opts_parser(
>>   	case N_FTYPE:
>>   		cli->sb_feat.dirftype = getnum(value, opts, subopt);
>>   		break;
>> +	case N_DELATTR:
>> +		cli->sb_feat.delattr = getnum(value, &nopts, N_DELATTR);
>> +		break;
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -1988,6 +2000,14 @@ _("reflink not supported without CRC support\n"));
>>   		cli->sb_feat.reflink = false;
>>   	}
>>   
>> +	if ((cli->sb_feat.delattr) &&
>> +	    cli->sb_feat.dir_version == 4) {
>> +		fprintf(stderr,
>> +_("delayed attributes not supported on v4 filesystems\n"));
> 
> I think this should move a few lines up to the big batch of code that
> turns off all the V5 features if crcs aren't enabled.
> 
> TBH this should silently turn off delattrs unless the admin explicitly
> enabled them, because one day this will be enabled by default.
Ok, I see it.  Will do.

> 
> --D
> 
>> +		usage();
>> +		cli->sb_feat.delattr = false;
>> +	}
>> +
>>   	if ((cli->fsx.fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
>>   	    !cli->sb_feat.reflink) {
>>   		fprintf(stderr,
>> @@ -2953,6 +2973,8 @@ sb_set_features(
>>   		sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_RMAPBT;
>>   	if (fp->reflink)
>>   		sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_REFLINK;
>> +	if (fp->delattr)
>> +		sbp->sb_features_log_incompat |= XFS_SB_FEAT_INCOMPAT_LOG_DELATTR;
>>   
>>   	/*
>>   	 * Sparse inode chunk support has two main inode alignment requirements.
>> -- 
>> 2.7.4
>>