diff mbox

[18/21] xfsprogs: Add parent pointer flag to cmd

Message ID 1525754479-12177-19-git-send-email-allison.henderson@oracle.com (mailing list archive)
State Superseded
Headers show

Commit Message

Allison Henderson May 8, 2018, 4:41 a.m. UTC 
Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
---
 mkfs/xfs_mkfs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong May 8, 2018, 5:25 p.m. UTC | #1 
On Mon, May 07, 2018 at 09:41:16PM -0700, Allison Henderson wrote:
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>

Needs commit message:

mkfs: enable formatting with parent pointers

Wire up parent pointer support in mkfs via the '-m parent' parameter.

Signed-off-by: <etc>

> ---
>  mkfs/xfs_mkfs.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 78d0ce5..554e1bf 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -129,6 +129,7 @@ enum {
>  	M_UUID,
>  	M_RMAPBT,
>  	M_REFLINK,
> +	M_PARENT,

Hmm, is this better off in the naming section?

e.g. "mkfs.xfs -n parent=1"

>  	M_MAX_OPTS,
>  };
>  
> @@ -663,6 +664,7 @@ struct opt_params mopts = {
>  		[M_UUID] = "uuid",
>  		[M_RMAPBT] = "rmapbt",
>  		[M_REFLINK] = "reflink",
> +		[M_PARENT] = "parent",
>  	},
>  	.subopt_params = {
>  		{ .index = M_CRC,
> @@ -693,6 +695,13 @@ struct opt_params mopts = {
>  		  .maxval = 1,
>  		  .defaultval = 1,
>  		},
> +		{ .index = M_PARENT,
> +		  .conflicts = { {NULL, LAST_CONFLICT } },
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 0,

This (misleadingly named) field is the default value if you pass the
argument without explicitly assigning a value, i.e.

# mkfs.xfs -m parent /dev/sda

sets parent to whatever defaultval is.  In this case you'd get no parent
pointers, which is a little surprising.

And, uh, seeing as people keep getting this wrong maybe we should rename
it?

> +		},
> +
>  	},
>  };
>  
> @@ -865,7 +874,7 @@ usage( void )
>  {
>  	fprintf(stderr, _("Usage: %s\n\
>  /* blocksize */		[-b size=num]\n\
> -/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
> +/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1,parent=0|1]\n\
>  /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
>  			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
>  			    sectsize=num\n\
> @@ -1586,6 +1595,9 @@ meta_opts_parser(
>  	case M_REFLINK:
>  		cli->sb_feat.reflink = getnum(value, opts, subopt);
>  		break;
> +	case M_PARENT:
> +		cli->sb_feat.parent_pointers = getnum(value, &mopts, M_PARENT);
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -2887,6 +2899,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->parent_pointers)
> +		sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_PARENT;

Otherwise looks ok to me....

--D

>  
>  	/*
>  	 * Sparse inode chunk support has two main inode alignment requirements.
> -- 
> 2.7.4
> 
> --
> 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
--
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
Allison Henderson May 8, 2018, 7:02 p.m. UTC | #2 
On 05/08/2018 10:25 AM, Darrick J. Wong wrote:
> On Mon, May 07, 2018 at 09:41:16PM -0700, Allison Henderson wrote:
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Needs commit message:
>
> mkfs: enable formatting with parent pointers
>
> Wire up parent pointer support in mkfs via the '-m parent' parameter.
>
> Signed-off-by: <etc>
Ok, will update
>> ---
>>   mkfs/xfs_mkfs.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 78d0ce5..554e1bf 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -129,6 +129,7 @@ enum {
>>   	M_UUID,
>>   	M_RMAPBT,
>>   	M_REFLINK,
>> +	M_PARENT,
> Hmm, is this better off in the naming section?
>
> e.g. "mkfs.xfs -n parent=1"
I suppose it could fit there too if that's preferred.  I wasnt really 
sure which section it fit best to :-)
>>   	M_MAX_OPTS,
>>   };
>>   
>> @@ -663,6 +664,7 @@ struct opt_params mopts = {
>>   		[M_UUID] = "uuid",
>>   		[M_RMAPBT] = "rmapbt",
>>   		[M_REFLINK] = "reflink",
>> +		[M_PARENT] = "parent",
>>   	},
>>   	.subopt_params = {
>>   		{ .index = M_CRC,
>> @@ -693,6 +695,13 @@ struct opt_params mopts = {
>>   		  .maxval = 1,
>>   		  .defaultval = 1,
>>   		},
>> +		{ .index = M_PARENT,
>> +		  .conflicts = { {NULL, LAST_CONFLICT } },
>> +		  .minval = 0,
>> +		  .maxval = 1,
>> +		  .defaultval = 0,
> This (misleadingly named) field is the default value if you pass the
> argument without explicitly assigning a value, i.e.
>
> # mkfs.xfs -m parent /dev/sda
>
> sets parent to whatever defaultval is.  In this case you'd get no parent
> pointers, which is a little surprising.
>
> And, uh, seeing as people keep getting this wrong maybe we should rename
> it?
Oh I see.  That is a little odd, is there really a better name though?  
Maybe just a comment or something might make it more clear.
>> +		},
>> +
>>   	},
>>   };
>>   
>> @@ -865,7 +874,7 @@ usage( void )
>>   {
>>   	fprintf(stderr, _("Usage: %s\n\
>>   /* blocksize */		[-b size=num]\n\
>> -/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
>> +/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1,parent=0|1]\n\
>>   /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
>>   			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
>>   			    sectsize=num\n\
>> @@ -1586,6 +1595,9 @@ meta_opts_parser(
>>   	case M_REFLINK:
>>   		cli->sb_feat.reflink = getnum(value, opts, subopt);
>>   		break;
>> +	case M_PARENT:
>> +		cli->sb_feat.parent_pointers = getnum(value, &mopts, M_PARENT);
>> +		break;
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -2887,6 +2899,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->parent_pointers)
>> +		sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_PARENT;
> Otherwise looks ok to me....
>
> --D
Alrighty, thx!
>>   
>>   	/*
>>   	 * Sparse inode chunk support has two main inode alignment requirements.
>> -- 
>> 2.7.4
>>
>> --
>> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=2DFx03tmCKRPVndQLwf1P1u0PChkNowVFyXqzZ_arE8&s=JiwNEPlNO54aLxRtPd9mGL_lKU8dpX4T6ECMmDQhc70&e=
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=2DFx03tmCKRPVndQLwf1P1u0PChkNowVFyXqzZ_arE8&s=JiwNEPlNO54aLxRtPd9mGL_lKU8dpX4T6ECMmDQhc70&e=

--
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
Darrick J. Wong May 8, 2018, 10:44 p.m. UTC | #3 
On Tue, May 08, 2018 at 12:02:29PM -0700, Allison Henderson wrote:
> On 05/08/2018 10:25 AM, Darrick J. Wong wrote:
> > On Mon, May 07, 2018 at 09:41:16PM -0700, Allison Henderson wrote:
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > Needs commit message:
> > 
> > mkfs: enable formatting with parent pointers
> > 
> > Wire up parent pointer support in mkfs via the '-m parent' parameter.
> > 
> > Signed-off-by: <etc>
> Ok, will update
> > > ---
> > >   mkfs/xfs_mkfs.c | 16 +++++++++++++++-
> > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > index 78d0ce5..554e1bf 100644
> > > --- a/mkfs/xfs_mkfs.c
> > > +++ b/mkfs/xfs_mkfs.c
> > > @@ -129,6 +129,7 @@ enum {
> > >   	M_UUID,
> > >   	M_RMAPBT,
> > >   	M_REFLINK,
> > > +	M_PARENT,
> > Hmm, is this better off in the naming section?
> > 
> > e.g. "mkfs.xfs -n parent=1"
> I suppose it could fit there too if that's preferred.  I wasnt really sure
> which section it fit best to :-)

<shrug> -n is where we put the other naming and directory related stuff.

> > >   	M_MAX_OPTS,
> > >   };
> > > @@ -663,6 +664,7 @@ struct opt_params mopts = {
> > >   		[M_UUID] = "uuid",
> > >   		[M_RMAPBT] = "rmapbt",
> > >   		[M_REFLINK] = "reflink",
> > > +		[M_PARENT] = "parent",
> > >   	},
> > >   	.subopt_params = {
> > >   		{ .index = M_CRC,
> > > @@ -693,6 +695,13 @@ struct opt_params mopts = {
> > >   		  .maxval = 1,
> > >   		  .defaultval = 1,
> > >   		},
> > > +		{ .index = M_PARENT,
> > > +		  .conflicts = { {NULL, LAST_CONFLICT } },
> > > +		  .minval = 0,
> > > +		  .maxval = 1,
> > > +		  .defaultval = 0,
> > This (misleadingly named) field is the default value if you pass the
> > argument without explicitly assigning a value, i.e.
> > 
> > # mkfs.xfs -m parent /dev/sda
> > 
> > sets parent to whatever defaultval is.  In this case you'd get no parent
> > pointers, which is a little surprising.
> > 
> > And, uh, seeing as people keep getting this wrong maybe we should rename
> > it?
> Oh I see.  That is a little odd, is there really a better name though? 
> Maybe just a comment or something might make it more clear.

"implied_default"
"implicit_default"
"valueIfNotExplicitlySpecified"
"m_thevalueassignedifnovalueisgiven"

(No, please not those last two...)

--D

> > > +		},
> > > +
> > >   	},
> > >   };
> > > @@ -865,7 +874,7 @@ usage( void )
> > >   {
> > >   	fprintf(stderr, _("Usage: %s\n\
> > >   /* blocksize */		[-b size=num]\n\
> > > -/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
> > > +/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1,parent=0|1]\n\
> > >   /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
> > >   			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
> > >   			    sectsize=num\n\
> > > @@ -1586,6 +1595,9 @@ meta_opts_parser(
> > >   	case M_REFLINK:
> > >   		cli->sb_feat.reflink = getnum(value, opts, subopt);
> > >   		break;
> > > +	case M_PARENT:
> > > +		cli->sb_feat.parent_pointers = getnum(value, &mopts, M_PARENT);
> > > +		break;
> > >   	default:
> > >   		return -EINVAL;
> > >   	}
> > > @@ -2887,6 +2899,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->parent_pointers)
> > > +		sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_PARENT;
> > Otherwise looks ok to me....
> > 
> > --D
> Alrighty, thx!
> > >   	/*
> > >   	 * Sparse inode chunk support has two main inode alignment requirements.
> > > -- 
> > > 2.7.4
> > > 
> > > --
> > > 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=2DFx03tmCKRPVndQLwf1P1u0PChkNowVFyXqzZ_arE8&s=JiwNEPlNO54aLxRtPd9mGL_lKU8dpX4T6ECMmDQhc70&e=
> > --
> > 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=2DFx03tmCKRPVndQLwf1P1u0PChkNowVFyXqzZ_arE8&s=JiwNEPlNO54aLxRtPd9mGL_lKU8dpX4T6ECMmDQhc70&e=
> 
> --
> 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
--
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
diff mbox

Patch

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 78d0ce5..554e1bf 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -129,6 +129,7 @@  enum {
 	M_UUID,
 	M_RMAPBT,
 	M_REFLINK,
+	M_PARENT,
 	M_MAX_OPTS,
 };
 
@@ -663,6 +664,7 @@  struct opt_params mopts = {
 		[M_UUID] = "uuid",
 		[M_RMAPBT] = "rmapbt",
 		[M_REFLINK] = "reflink",
+		[M_PARENT] = "parent",
 	},
 	.subopt_params = {
 		{ .index = M_CRC,
@@ -693,6 +695,13 @@  struct opt_params mopts = {
 		  .maxval = 1,
 		  .defaultval = 1,
 		},
+		{ .index = M_PARENT,
+		  .conflicts = { {NULL, LAST_CONFLICT } },
+		  .minval = 0,
+		  .maxval = 1,
+		  .defaultval = 0,
+		},
+
 	},
 };
 
@@ -865,7 +874,7 @@  usage( void )
 {
 	fprintf(stderr, _("Usage: %s\n\
 /* blocksize */		[-b size=num]\n\
-/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
+/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1,parent=0|1]\n\
 /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
 			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
 			    sectsize=num\n\
@@ -1586,6 +1595,9 @@  meta_opts_parser(
 	case M_REFLINK:
 		cli->sb_feat.reflink = getnum(value, opts, subopt);
 		break;
+	case M_PARENT:
+		cli->sb_feat.parent_pointers = getnum(value, &mopts, M_PARENT);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -2887,6 +2899,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->parent_pointers)
+		sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_PARENT;
 
 	/*
 	 * Sparse inode chunk support has two main inode alignment requirements.