Re: [PATCH 1/2] fs: hoist BTRFS_IOC_[SG]ET_FSLABEL to vfs

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Andreas Dilger <adilger@dilger.ca>
Cc: Eric Sandeen <sandeen@sandeen.net>,
	Eric Sandeen <sandeen@redhat.com>,
	fsdevel <linux-fsdevel@vger.kernel.org>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH 1/2] fs: hoist BTRFS_IOC_[SG]ET_FSLABEL to vfs
Date: Wed, 9 May 2018 10:26:13 -0700	[thread overview]
Message-ID: <20180509172613.GB25312@magnolia> (raw)
In-Reply-To: <E100AC43-CB61-4E90-B605-4E1B77AD3268@dilger.ca>

On Wed, May 09, 2018 at 11:15:46AM -0600, Andreas Dilger wrote:
> On May 9, 2018, at 10:10 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > 
> > On Wed, May 09, 2018 at 11:01:21AM -0500, Eric Sandeen wrote:
> >> Move the btrfs label ioctls up to the vfs for general use.
> >> 
> >> This retains 256 chars as the maximum size through the interface, which
> >> is the btrfs limit and AFAIK exceeds any other filesystem's maximum
> >> label size.
> >> 
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >> 
> >> Let the bikeshedding on the exact ioctl name begin ;)
> >> 
> >> fs/btrfs/ioctl.c           | 8 ++++----
> >> include/uapi/linux/btrfs.h | 6 ++----
> >> include/uapi/linux/fs.h    | 8 ++++++--
> >> 3 files changed, 12 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >> index 632e26d..2dd4cdf 100644
> >> --- a/fs/btrfs/ioctl.c
> >> +++ b/fs/btrfs/ioctl.c
> >> @@ -5444,6 +5444,10 @@ long btrfs_ioctl(struct file *file, unsigned int
> >> 		return btrfs_ioctl_setflags(file, argp);
> >> 	case FS_IOC_GETVERSION:
> >> 		return btrfs_ioctl_getversion(file, argp);
> >> +	case FS_IOC_GETFSLABEL:
> >> +		return btrfs_ioctl_get_fslabel(file, argp);
> >> +	case FS_IOC_SETFSLABEL:
> >> +		return btrfs_ioctl_set_fslabel(file, argp);
> >> 	case FITRIM:
> >> 		return btrfs_ioctl_fitrim(file, argp);
> >> 	case BTRFS_IOC_SNAP_CREATE:
> >> @@ -5555,10 +5559,6 @@ long btrfs_ioctl(struct file *file, unsigned int
> >> 		return btrfs_ioctl_quota_rescan_wait(file, argp);
> >> 	case BTRFS_IOC_DEV_REPLACE:
> >> 		return btrfs_ioctl_dev_replace(fs_info, argp);
> >> -	case BTRFS_IOC_GET_FSLABEL:
> >> -		return btrfs_ioctl_get_fslabel(file, argp);
> >> -	case BTRFS_IOC_SET_FSLABEL:
> >> -		return btrfs_ioctl_set_fslabel(file, argp);
> >> 	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
> >> 		return btrfs_ioctl_get_supported_features(argp);
> >> 	case BTRFS_IOC_GET_FEATURES:
> >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> >> index c8d99b9..ec611c8 100644
> >> --- a/include/uapi/linux/btrfs.h
> >> +++ b/include/uapi/linux/btrfs.h
> >> @@ -823,10 +823,8 @@ enum btrfs_err_code {
> >> #define BTRFS_IOC_QUOTA_RESCAN_STATUS _IOR(BTRFS_IOCTL_MAGIC, 45, \
> >> 			       struct btrfs_ioctl_quota_rescan_args)
> >> #define BTRFS_IOC_QUOTA_RESCAN_WAIT _IO(BTRFS_IOCTL_MAGIC, 46)
> >> -#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
> >> -				   char[BTRFS_LABEL_SIZE])
> >> -#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
> >> -				   char[BTRFS_LABEL_SIZE])
> >> +#define BTRFS_IOC_GET_FSLABEL 	FS_IOC_GETFSLABEL
> >> +#define BTRFS_IOC_SET_FSLABEL	FS_IOC_SETFSLABEL
> >> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
> >> 				      struct btrfs_ioctl_get_dev_stats)
> >> #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
> >> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> >> index d2a8313..1df3707 100644
> >> --- a/include/uapi/linux/fs.h
> >> +++ b/include/uapi/linux/fs.h
> >> @@ -242,6 +242,8 @@ struct fsxattr {
> >> #define FICLONERANGE	_IOW(0x94, 13, struct file_clone_range)
> >> #define FIDEDUPERANGE	_IOWR(0x94, 54, struct file_dedupe_range)
> >> 
> >> +#define FSLABEL_MAX 256	/* Max chars for the interface; each fs may differ */
> >> +
> >> #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
> >> #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
> >> #define	FS_IOC_GETVERSION		_IOR('v', 1, long)
> >> @@ -251,8 +253,10 @@ struct fsxattr {
> >> #define FS_IOC32_SETFLAGS		_IOW('f', 2, int)
> >> #define FS_IOC32_GETVERSION		_IOR('v', 1, int)
> >> #define FS_IOC32_SETVERSION		_IOW('v', 2, int)
> >> -#define FS_IOC_FSGETXATTR		_IOR ('X', 31, struct fsxattr)
> >> -#define FS_IOC_FSSETXATTR		_IOW ('X', 32, struct fsxattr)
> >> +#define FS_IOC_FSGETXATTR		_IOR('X', 31, struct fsxattr)
> >> +#define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
> > 
> > Separate patch for whitespace cleanup.
> 
> Really?  I've heard Ted complain the other way, that whitespace cleanup
> by itself is useless and should only be done as part of other changes.
> As long as it is not overwhelming the rest of the patch I don't see an
> issue with a minor improvement being part of another patch.

I really only meant this as: put the whitespace changes in a second
patch after this one so that we don't have a patch that implements two
different changes, but fmeh, tired of discussing this.

--D

> Otherwise, no bikeshedding from me.  Looks very reasonable, and the 256-char
> limit is definitely large enough IMHO (also matches the normal filename size
> limit, so if the label is used as a pathname component there are no added
> restrictions).
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
> Cheers, Andreas
> 
> >> +#define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
> >> +#define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> > 
> > Looks ok otherwise,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > --D
> > 
> >> 
> >> /*
> >>  * File system encryption support
> >> --
> >> 1.8.3.1
> >> 
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 