[PATCH 0/5] xfs: add online relabel capabilities

[PATCH 0/5] xfs: add online relabel capabilities

[Eric Sandeen]

Sending V1 just to the xfs list for xfs comments, and assuming
at least a V2 will emerge; I'll cc: fsdevel & btrfs on that one.

This implements online label set/get ioctls for xfs, following the
btrfs example.  We've had at least one request for this, and Chris Mason
tells me he's heard similar requests, and apparently even Darrick wished
for this once.  :)

Patch5 is the xfs_io userspace patch, once this is all sorted I'll add a
wrapper to xfs_admin as well.

This can also be tested w/ the "btrfs filesystem label" command since it
re-uses their ioctl.

Thanks,
-Eric

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

[Eric Sandeen]

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>
---
 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_GET_FSLABEL:
+		return btrfs_ioctl_get_fslabel(file, argp);
+	case FS_IOC_SET_FSLABEL:
+		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_GET_FSLABEL
+#define BTRFS_IOC_SET_FSLABEL	FS_IOC_SET_FSLABEL
 #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)
+#define FS_IOC_GET_FSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
+#define FS_IOC_SET_FSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
 
 /*
  * File system encryption support
-- 
1.8.3.1

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

[Darrick J. Wong]

On Mon, Apr 30, 2018 at 10:42:44AM -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>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  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_GET_FSLABEL:
> +		return btrfs_ioctl_get_fslabel(file, argp);
> +	case FS_IOC_SET_FSLABEL:
> +		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_GET_FSLABEL
> +#define BTRFS_IOC_SET_FSLABEL	FS_IOC_SET_FSLABEL
>  #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)
> +#define FS_IOC_GET_FSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
> +#define FS_IOC_SET_FSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
>  
>  /*
>   * File system encryption support
> -- 
> 1.8.3.1
> 
> --
> 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

[PATCH 2/5] xfs: factor out secondary superblock updates

[Eric Sandeen]

growfs rewrites all secondary supers, and relabel should do so as well;
factor out the code which does this for use by both operations.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_fsops.c | 130 +++++++++++++++++++++++++++++++----------------------
 fs/xfs/xfs_fsops.h |   2 +
 2 files changed, 79 insertions(+), 53 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 5237927..8f2a73a 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -71,6 +71,79 @@
 	return bp;
 }
 
+/*
+ * Copy the contents of the primary super to all backup supers.
+ * %agcount is currently existing AGs
+ * %new_ags is the number of new AGs to write out if we're growing the
+ * filesystem.
+ * We can't use mp->m_sb.sb_agcount here because growfs already updated it.
+ */
+int
+xfs_update_secondary_supers(
+	xfs_mount_t		*mp,
+	xfs_agnumber_t		agcount,
+	xfs_agnumber_t		new_ags)
+{
+	int			error, saved_error;
+	xfs_agnumber_t		agno;
+	xfs_buf_t		*bp;
+
+	error = saved_error = 0;
+
+	/* update secondary superblocks. */
+	for (agno = 1; agno < agcount + new_ags; agno++) {
+		error = 0;
+		/*
+		 * new secondary superblocks need to be zeroed, not read from
+		 * disk as the contents of the new area we are growing into is
+		 * completely unknown.
+		 */
+		if (agno < agcount) {
+			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
+				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
+				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
+				  &xfs_sb_buf_ops);
+		} else {
+			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
+				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
+				  XFS_FSS_TO_BB(mp, 1), 0);
+			if (bp) {
+				bp->b_ops = &xfs_sb_buf_ops;
+				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
+			} else
+				error = -ENOMEM;
+		}
+
+		/*
+		 * If we get an error reading or writing alternate superblocks,
+		 * continue.  xfs_repair chooses the "best" superblock based
+		 * on most matches; if we break early, we'll leave more
+		 * superblocks un-updated than updated, and xfs_repair may
+		 * pick them over the properly-updated primary.
+		 */
+		if (error) {
+			xfs_warn(mp,
+		"error %d reading secondary superblock for ag %d",
+				error, agno);
+			saved_error = error;
+			continue;
+		}
+		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
+
+		error = xfs_bwrite(bp);
+		xfs_buf_relse(bp);
+		if (error) {
+			xfs_warn(mp,
+		"write error %d updating secondary superblock for ag %d",
+				error, agno);
+			saved_error = error;
+			continue;
+		}
+	}
+
+	return saved_error ? saved_error : error;
+}
+
 static int
 xfs_growfs_data_private(
 	xfs_mount_t		*mp,		/* mount point for filesystem */
@@ -86,7 +159,7 @@
 	xfs_buf_t		*bp;
 	int			bucket;
 	int			dpct;
-	int			error, saved_error = 0;
+	int			error;
 	xfs_agnumber_t		nagcount;
 	xfs_agnumber_t		nagimax = 0;
 	xfs_rfsblock_t		nb, nb_mod;
@@ -557,59 +630,10 @@
 	if (error && error != -ENOSPC)
 		goto out;
 
-	/* update secondary superblocks. */
-	for (agno = 1; agno < nagcount; agno++) {
-		error = 0;
-		/*
-		 * new secondary superblocks need to be zeroed, not read from
-		 * disk as the contents of the new area we are growing into is
-		 * completely unknown.
-		 */
-		if (agno < oagcount) {
-			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
-				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
-				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
-				  &xfs_sb_buf_ops);
-		} else {
-			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
-				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
-				  XFS_FSS_TO_BB(mp, 1), 0);
-			if (bp) {
-				bp->b_ops = &xfs_sb_buf_ops;
-				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
-			} else
-				error = -ENOMEM;
-		}
-
-		/*
-		 * If we get an error reading or writing alternate superblocks,
-		 * continue.  xfs_repair chooses the "best" superblock based
-		 * on most matches; if we break early, we'll leave more
-		 * superblocks un-updated than updated, and xfs_repair may
-		 * pick them over the properly-updated primary.
-		 */
-		if (error) {
-			xfs_warn(mp,
-		"error %d reading secondary superblock for ag %d",
-				error, agno);
-			saved_error = error;
-			continue;
-		}
-		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
-
-		error = xfs_bwrite(bp);
-		xfs_buf_relse(bp);
-		if (error) {
-			xfs_warn(mp,
-		"write error %d updating secondary superblock for ag %d",
-				error, agno);
-			saved_error = error;
-			continue;
-		}
-	}
-
+	/* Copy new geometry to all backup superblocks, old & new */
+	error = xfs_update_secondary_supers(mp, oagcount, nagcount - oagcount);
  out:
-	return saved_error ? saved_error : error;
+	return error;
 
  error0:
 	xfs_trans_cancel(tp);
diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h
index 20484ed..ffc5377 100644
--- a/fs/xfs/xfs_fsops.h
+++ b/fs/xfs/xfs_fsops.h
@@ -18,6 +18,8 @@
 #ifndef __XFS_FSOPS_H__
 #define	__XFS_FSOPS_H__
 
+extern int xfs_update_secondary_supers(xfs_mount_t *mp, xfs_agnumber_t agcount,
+				xfs_agnumber_t new_ags);
 extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in);
 extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in);
 extern int xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt);
-- 
1.8.3.1

Re: [PATCH 2/5] xfs: factor out secondary superblock updates

[Brian Foster]

On Mon, Apr 30, 2018 at 10:43:55AM -0500, Eric Sandeen wrote:
> growfs rewrites all secondary supers, and relabel should do so as well;
> factor out the code which does this for use by both operations.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

FYI I think Dave has some patches in progress to rework growfs. It might
be useful to coordinate on the refactoring bits, at least..

Brian

>  fs/xfs/xfs_fsops.c | 130 +++++++++++++++++++++++++++++++----------------------
>  fs/xfs/xfs_fsops.h |   2 +
>  2 files changed, 79 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 5237927..8f2a73a 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -71,6 +71,79 @@
>  	return bp;
>  }
>  
> +/*
> + * Copy the contents of the primary super to all backup supers.
> + * %agcount is currently existing AGs
> + * %new_ags is the number of new AGs to write out if we're growing the
> + * filesystem.
> + * We can't use mp->m_sb.sb_agcount here because growfs already updated it.
> + */
> +int
> +xfs_update_secondary_supers(
> +	xfs_mount_t		*mp,
> +	xfs_agnumber_t		agcount,
> +	xfs_agnumber_t		new_ags)
> +{
> +	int			error, saved_error;
> +	xfs_agnumber_t		agno;
> +	xfs_buf_t		*bp;
> +
> +	error = saved_error = 0;
> +
> +	/* update secondary superblocks. */
> +	for (agno = 1; agno < agcount + new_ags; agno++) {
> +		error = 0;
> +		/*
> +		 * new secondary superblocks need to be zeroed, not read from
> +		 * disk as the contents of the new area we are growing into is
> +		 * completely unknown.
> +		 */
> +		if (agno < agcount) {
> +			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
> +				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> +				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
> +				  &xfs_sb_buf_ops);
> +		} else {
> +			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
> +				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> +				  XFS_FSS_TO_BB(mp, 1), 0);
> +			if (bp) {
> +				bp->b_ops = &xfs_sb_buf_ops;
> +				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
> +			} else
> +				error = -ENOMEM;
> +		}
> +
> +		/*
> +		 * If we get an error reading or writing alternate superblocks,
> +		 * continue.  xfs_repair chooses the "best" superblock based
> +		 * on most matches; if we break early, we'll leave more
> +		 * superblocks un-updated than updated, and xfs_repair may
> +		 * pick them over the properly-updated primary.
> +		 */
> +		if (error) {
> +			xfs_warn(mp,
> +		"error %d reading secondary superblock for ag %d",
> +				error, agno);
> +			saved_error = error;
> +			continue;
> +		}
> +		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
> +
> +		error = xfs_bwrite(bp);
> +		xfs_buf_relse(bp);
> +		if (error) {
> +			xfs_warn(mp,
> +		"write error %d updating secondary superblock for ag %d",
> +				error, agno);
> +			saved_error = error;
> +			continue;
> +		}
> +	}
> +
> +	return saved_error ? saved_error : error;
> +}
> +
>  static int
>  xfs_growfs_data_private(
>  	xfs_mount_t		*mp,		/* mount point for filesystem */
> @@ -86,7 +159,7 @@
>  	xfs_buf_t		*bp;
>  	int			bucket;
>  	int			dpct;
> -	int			error, saved_error = 0;
> +	int			error;
>  	xfs_agnumber_t		nagcount;
>  	xfs_agnumber_t		nagimax = 0;
>  	xfs_rfsblock_t		nb, nb_mod;
> @@ -557,59 +630,10 @@
>  	if (error && error != -ENOSPC)
>  		goto out;
>  
> -	/* update secondary superblocks. */
> -	for (agno = 1; agno < nagcount; agno++) {
> -		error = 0;
> -		/*
> -		 * new secondary superblocks need to be zeroed, not read from
> -		 * disk as the contents of the new area we are growing into is
> -		 * completely unknown.
> -		 */
> -		if (agno < oagcount) {
> -			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
> -				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> -				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
> -				  &xfs_sb_buf_ops);
> -		} else {
> -			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
> -				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> -				  XFS_FSS_TO_BB(mp, 1), 0);
> -			if (bp) {
> -				bp->b_ops = &xfs_sb_buf_ops;
> -				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
> -			} else
> -				error = -ENOMEM;
> -		}
> -
> -		/*
> -		 * If we get an error reading or writing alternate superblocks,
> -		 * continue.  xfs_repair chooses the "best" superblock based
> -		 * on most matches; if we break early, we'll leave more
> -		 * superblocks un-updated than updated, and xfs_repair may
> -		 * pick them over the properly-updated primary.
> -		 */
> -		if (error) {
> -			xfs_warn(mp,
> -		"error %d reading secondary superblock for ag %d",
> -				error, agno);
> -			saved_error = error;
> -			continue;
> -		}
> -		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
> -
> -		error = xfs_bwrite(bp);
> -		xfs_buf_relse(bp);
> -		if (error) {
> -			xfs_warn(mp,
> -		"write error %d updating secondary superblock for ag %d",
> -				error, agno);
> -			saved_error = error;
> -			continue;
> -		}
> -	}
> -
> +	/* Copy new geometry to all backup superblocks, old & new */
> +	error = xfs_update_secondary_supers(mp, oagcount, nagcount - oagcount);
>   out:
> -	return saved_error ? saved_error : error;
> +	return error;
>  
>   error0:
>  	xfs_trans_cancel(tp);
> diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h
> index 20484ed..ffc5377 100644
> --- a/fs/xfs/xfs_fsops.h
> +++ b/fs/xfs/xfs_fsops.h
> @@ -18,6 +18,8 @@
>  #ifndef __XFS_FSOPS_H__
>  #define	__XFS_FSOPS_H__
>  
> +extern int xfs_update_secondary_supers(xfs_mount_t *mp, xfs_agnumber_t agcount,
> +				xfs_agnumber_t new_ags);
>  extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in);
>  extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in);
>  extern int xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt);
> -- 
> 1.8.3.1
> 
> --
> 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

Re: [PATCH 2/5] xfs: factor out secondary superblock updates

[Eric Sandeen]

On 5/1/18 9:17 AM, Brian Foster wrote:
> On Mon, Apr 30, 2018 at 10:43:55AM -0500, Eric Sandeen wrote:
>> growfs rewrites all secondary supers, and relabel should do so as well;
>> factor out the code which does this for use by both operations.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
> 
> FYI I think Dave has some patches in progress to rework growfs. It might
> be useful to coordinate on the refactoring bits, at least..

Yup.  Growfs definitely needs a refactor and I know he's working on that,
but I haven't seen Dave's patches.  Dave?  :)

-Eric

Re: [PATCH 2/5] xfs: factor out secondary superblock updates

[Darrick J. Wong]

On Mon, Apr 30, 2018 at 10:43:55AM -0500, Eric Sandeen wrote:
> growfs rewrites all secondary supers, and relabel should do so as well;
> factor out the code which does this for use by both operations.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_fsops.c | 130 +++++++++++++++++++++++++++++++----------------------
>  fs/xfs/xfs_fsops.h |   2 +
>  2 files changed, 79 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 5237927..8f2a73a 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -71,6 +71,79 @@
>  	return bp;
>  }
>  
> +/*
> + * Copy the contents of the primary super to all backup supers.
> + * %agcount is currently existing AGs
> + * %new_ags is the number of new AGs to write out if we're growing the
> + * filesystem.
> + * We can't use mp->m_sb.sb_agcount here because growfs already updated it.
> + */
> +int
> +xfs_update_secondary_supers(
> +	xfs_mount_t		*mp,
> +	xfs_agnumber_t		agcount,
> +	xfs_agnumber_t		new_ags)
> +{
> +	int			error, saved_error;
> +	xfs_agnumber_t		agno;
> +	xfs_buf_t		*bp;
> +
> +	error = saved_error = 0;
> +
> +	/* update secondary superblocks. */
> +	for (agno = 1; agno < agcount + new_ags; agno++) {
> +		error = 0;
> +		/*
> +		 * new secondary superblocks need to be zeroed, not read from
> +		 * disk as the contents of the new area we are growing into is
> +		 * completely unknown.
> +		 */
> +		if (agno < agcount) {
> +			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
> +				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> +				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
> +				  &xfs_sb_buf_ops);
> +		} else {
> +			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
> +				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> +				  XFS_FSS_TO_BB(mp, 1), 0);
> +			if (bp) {
> +				bp->b_ops = &xfs_sb_buf_ops;
> +				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
> +			} else
> +				error = -ENOMEM;
> +		}
> +
> +		/*
> +		 * If we get an error reading or writing alternate superblocks,
> +		 * continue.  xfs_repair chooses the "best" superblock based
> +		 * on most matches; if we break early, we'll leave more
> +		 * superblocks un-updated than updated, and xfs_repair may
> +		 * pick them over the properly-updated primary.
> +		 */
> +		if (error) {
> +			xfs_warn(mp,
> +		"error %d reading secondary superblock for ag %d",
> +				error, agno);
> +			saved_error = error;
> +			continue;
> +		}
> +		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
> +
> +		error = xfs_bwrite(bp);
> +		xfs_buf_relse(bp);
> +		if (error) {
> +			xfs_warn(mp,
> +		"write error %d updating secondary superblock for ag %d",
> +				error, agno);
> +			saved_error = error;
> +			continue;
> +		}
> +	}
> +
> +	return saved_error ? saved_error : error;
> +}
> +
>  static int
>  xfs_growfs_data_private(
>  	xfs_mount_t		*mp,		/* mount point for filesystem */
> @@ -86,7 +159,7 @@
>  	xfs_buf_t		*bp;
>  	int			bucket;
>  	int			dpct;
> -	int			error, saved_error = 0;
> +	int			error;
>  	xfs_agnumber_t		nagcount;
>  	xfs_agnumber_t		nagimax = 0;
>  	xfs_rfsblock_t		nb, nb_mod;
> @@ -557,59 +630,10 @@
>  	if (error && error != -ENOSPC)
>  		goto out;
>  
> -	/* update secondary superblocks. */
> -	for (agno = 1; agno < nagcount; agno++) {
> -		error = 0;
> -		/*
> -		 * new secondary superblocks need to be zeroed, not read from
> -		 * disk as the contents of the new area we are growing into is
> -		 * completely unknown.
> -		 */
> -		if (agno < oagcount) {
> -			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
> -				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> -				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
> -				  &xfs_sb_buf_ops);
> -		} else {
> -			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
> -				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> -				  XFS_FSS_TO_BB(mp, 1), 0);
> -			if (bp) {
> -				bp->b_ops = &xfs_sb_buf_ops;
> -				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
> -			} else
> -				error = -ENOMEM;
> -		}
> -
> -		/*
> -		 * If we get an error reading or writing alternate superblocks,
> -		 * continue.  xfs_repair chooses the "best" superblock based
> -		 * on most matches; if we break early, we'll leave more
> -		 * superblocks un-updated than updated, and xfs_repair may
> -		 * pick them over the properly-updated primary.
> -		 */
> -		if (error) {
> -			xfs_warn(mp,
> -		"error %d reading secondary superblock for ag %d",
> -				error, agno);
> -			saved_error = error;
> -			continue;
> -		}
> -		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
> -
> -		error = xfs_bwrite(bp);
> -		xfs_buf_relse(bp);
> -		if (error) {
> -			xfs_warn(mp,
> -		"write error %d updating secondary superblock for ag %d",
> -				error, agno);
> -			saved_error = error;
> -			continue;
> -		}
> -	}
> -
> +	/* Copy new geometry to all backup superblocks, old & new */
> +	error = xfs_update_secondary_supers(mp, oagcount, nagcount - oagcount);
>   out:
> -	return saved_error ? saved_error : error;
> +	return error;
>  
>   error0:
>  	xfs_trans_cancel(tp);
> diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h
> index 20484ed..ffc5377 100644
> --- a/fs/xfs/xfs_fsops.h
> +++ b/fs/xfs/xfs_fsops.h
> @@ -18,6 +18,8 @@
>  #ifndef __XFS_FSOPS_H__
>  #define	__XFS_FSOPS_H__
>  
> +extern int xfs_update_secondary_supers(xfs_mount_t *mp, xfs_agnumber_t agcount,
> +				xfs_agnumber_t new_ags);
>  extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in);
>  extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in);
>  extern int xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt);
> -- 
> 1.8.3.1
> 
> --
> 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

[PATCH 3/5] xfs: move xfs_scrub_checkpoint_log to xfs_log_checkpoint for general use

[Eric Sandeen]

The upcoming online label patch will require forcing the superblock
change from the log to the actual on-disk superblock, so move this log
checkpoint function out for general use.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/scrub/common.c | 16 +---------------
 fs/xfs/scrub/common.h |  1 -
 fs/xfs/xfs_log.c      | 14 ++++++++++++++
 fs/xfs/xfs_log.h      |  1 +
 4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 8ed91d5..6044399 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -594,7 +594,7 @@ struct xfs_scrub_rmap_ownedby_info {
 	 * document why they need to do so.
 	 */
 	if (force_log) {
-		error = xfs_scrub_checkpoint_log(mp);
+		error = xfs_log_checkpoint(mp);
 		if (error)
 			return error;
 	}
@@ -606,20 +606,6 @@ struct xfs_scrub_rmap_ownedby_info {
 	return xfs_scrub_ag_init(sc, sc->sm->sm_agno, &sc->sa);
 }
 
-/* Push everything out of the log onto disk. */
-int
-xfs_scrub_checkpoint_log(
-	struct xfs_mount	*mp)
-{
-	int			error;
-
-	error = xfs_log_force(mp, XFS_LOG_SYNC);
-	if (error)
-		return error;
-	xfs_ail_push_all_sync(mp->m_ail);
-	return 0;
-}
-
 /*
  * Given an inode and the scrub control structure, grab either the
  * inode referenced in the control structure or the inode passed in.
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index deaf604..a88ae07 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -83,7 +83,6 @@ void xfs_scrub_fblock_set_warning(struct xfs_scrub_context *sc, int whichfork,
 		xfs_fileoff_t offset);
 
 void xfs_scrub_set_incomplete(struct xfs_scrub_context *sc);
-int xfs_scrub_checkpoint_log(struct xfs_mount *mp);
 
 /* Are we set up for a cross-referencing check? */
 bool xfs_scrub_should_check_xref(struct xfs_scrub_context *sc, int *error,
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2fcd9ed..326fbe8 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -4095,3 +4095,17 @@ struct xlog_ticket *
 
 	return valid;
 }
+
+/* Push everything out of the log onto disk. */
+int
+xfs_log_checkpoint(
+	struct xfs_mount	*mp)
+{
+	int			error;
+
+	error = xfs_log_force(mp, XFS_LOG_SYNC);
+	if (error)
+		return error;
+	xfs_ail_push_all_sync(mp->m_ail);
+	return 0;
+}
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index fa8ad31..df2d5d2 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -165,5 +165,6 @@ void	xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp,
 void	xfs_log_work_queue(struct xfs_mount *mp);
 void	xfs_log_quiesce(struct xfs_mount *mp);
 bool	xfs_log_check_lsn(struct xfs_mount *, xfs_lsn_t);
+int	xfs_log_checkpoint(struct xfs_mount *mp);
 
 #endif	/* __XFS_LOG_H__ */
-- 
1.8.3.1

Re: [PATCH 3/5] xfs: move xfs_scrub_checkpoint_log to xfs_log_checkpoint for general use

[Eric Sandeen]

On 4/30/18 10:45 AM, Eric Sandeen wrote:
> The upcoming online label patch will require forcing the superblock
> change from the log to the actual on-disk superblock, so move this log
> checkpoint function out for general use.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

self-nak / drop this one, not needed now.

-Eric

[PATCH 4/5] xfs: implement online get/set fs label

[Eric Sandeen]

The GET ioctl is trivial, just return the current label.

The SET ioctl is more involved:
It transactionally modifies the superblock to write a new filesystem
label to the primary super.

It then also checkpoints the log to disk so that the change lands in
block 0, invalidates any page cache that userspace might have previously
read, and updates all secondary superblocks as userspace relable does.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

nb: the growfs trylock is just cargo-culting; growfs also trylocks.  *shrug*

 fs/xfs/xfs_ioctl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 89fb1eb..12c6711 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -20,6 +20,7 @@
 #include "xfs_shared.h"
 #include "xfs_format.h"
 #include "xfs_log_format.h"
+#include "xfs_log.h"
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_inode.h"
@@ -1811,6 +1812,84 @@ struct getfsmap_info {
 	return error;
 }
 
+static int
+xfs_ioc_getlabel(
+	struct xfs_mount	*mp,
+	char			__user *label)
+{
+	struct xfs_sb		*sbp = &mp->m_sb;
+
+	/* Paranoia */
+	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
+
+	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
+		return -EFAULT;
+	return 0;
+}
+
+static int
+xfs_ioc_setlabel(
+	struct file		*filp,
+	struct xfs_mount	*mp,
+	char			__user *newlabel)
+{
+	struct address_space	*mapping;
+	struct xfs_sb		*sbp = &mp->m_sb;
+	char			label[FSLABEL_MAX];
+	size_t			len;
+	int			error;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(label, newlabel, FSLABEL_MAX))
+		return -EFAULT;
+	/*
+	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
+	 * smaller, at 12 bytes.
+	 * NB: The on disk label doesn't need to be null terminated.
+	 */
+	len = strnlen(label, FSLABEL_MAX);
+	if (len > sizeof(sbp->sb_fname)) {
+		xfs_warn(mp, "label %s is too long, max %zu chars",
+			label, sizeof(sbp->sb_fname));
+		return -EINVAL;
+	}
+
+	error = mnt_want_write_file(filp);
+	if (error)
+		return error;
+
+	spin_lock(&mp->m_sb_lock);
+	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
+	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
+	spin_unlock(&mp->m_sb_lock);
+
+	error = xfs_sync_sb(mp, true);
+	if (error)
+		goto out;
+
+	/* checkpoint the log to update primary super in fs itself */
+	error = xfs_log_checkpoint(mp);
+	if (error)
+		goto out;
+
+	/* invalidate any cached bdev page for userspace visibility */
+	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
+	error = invalidate_inode_pages2_range(mapping, 0, 0);
+	if (error)
+		goto out;
+
+	/* update the backup superblocks like userspace does */
+	if (mutex_trylock(&mp->m_growlock)) {
+		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
+		mutex_unlock(&mp->m_growlock);
+	}
+out:
+	mnt_drop_write_file(filp);
+	return error;
+}
+
 /*
  * Note: some of the ioctl's return positive numbers as a
  * byte count indicating success, such as readlink_by_handle.
@@ -1834,6 +1913,10 @@ struct getfsmap_info {
 	switch (cmd) {
 	case FITRIM:
 		return xfs_ioc_trim(mp, arg);
+	case FS_IOC_GET_FSLABEL:
+		return xfs_ioc_getlabel(mp, arg);
+	case FS_IOC_SET_FSLABEL:
+		return xfs_ioc_setlabel(filp, mp, arg);
 	case XFS_IOC_ALLOCSP:
 	case XFS_IOC_FREESP:
 	case XFS_IOC_RESVSP:
-- 
1.8.3.1

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Brian Foster]

On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
> The GET ioctl is trivial, just return the current label.
> 
> The SET ioctl is more involved:
> It transactionally modifies the superblock to write a new filesystem
> label to the primary super.
> 
> It then also checkpoints the log to disk so that the change lands in
> block 0, invalidates any page cache that userspace might have previously
> read, and updates all secondary superblocks as userspace relable does.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> nb: the growfs trylock is just cargo-culting; growfs also trylocks.  *shrug*
> 
>  fs/xfs/xfs_ioctl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 89fb1eb..12c6711 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -20,6 +20,7 @@
>  #include "xfs_shared.h"
>  #include "xfs_format.h"
>  #include "xfs_log_format.h"
> +#include "xfs_log.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_inode.h"
> @@ -1811,6 +1812,84 @@ struct getfsmap_info {
>  	return error;
>  }
>  
> +static int
> +xfs_ioc_getlabel(
> +	struct xfs_mount	*mp,
> +	char			__user *label)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +
> +	/* Paranoia */
> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> +
> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int
> +xfs_ioc_setlabel(
> +	struct file		*filp,
> +	struct xfs_mount	*mp,
> +	char			__user *newlabel)
> +{
> +	struct address_space	*mapping;
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +	char			label[FSLABEL_MAX];
> +	size_t			len;
> +	int			error;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
> +		return -EFAULT;
> +	/*
> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
> +	 * smaller, at 12 bytes.
> +	 * NB: The on disk label doesn't need to be null terminated.
> +	 */
> +	len = strnlen(label, FSLABEL_MAX);
> +	if (len > sizeof(sbp->sb_fname)) {
> +		xfs_warn(mp, "label %s is too long, max %zu chars",
> +			label, sizeof(sbp->sb_fname));
> +		return -EINVAL;
> +	}
> +
> +	error = mnt_want_write_file(filp);
> +	if (error)
> +		return error;
> +
> +	spin_lock(&mp->m_sb_lock);
> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> +	spin_unlock(&mp->m_sb_lock);
> +
> +	error = xfs_sync_sb(mp, true);
> +	if (error)
> +		goto out;

Is there a reason this all has to be synchronous in the first place, as
opposed to using xfs_log_sb() and committing a transaction for example?
Is it because we essentially don't have a way to recover if we crash
partway through? (Whatever the reason, a brief comment around why is
useful for future reference).

> +
> +	/* checkpoint the log to update primary super in fs itself */
> +	error = xfs_log_checkpoint(mp);
> +	if (error)
> +		goto out;
> +

xfs_sync_sb() with wait == true marks the transaction sync, which does
the log force part (but doesn't push the ail) based on the lsn of the
commit. Also note that you should be able to push the AIL based on the
lsn in the log item, which is set on AIL insertion, as opposed to doing
a full AIL push as well.

> +	/* invalidate any cached bdev page for userspace visibility */
> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
> +	if (error)
> +		goto out;

A more detailed comment on why this is necessary would be helpful. :)

> +
> +	/* update the backup superblocks like userspace does */
> +	if (mutex_trylock(&mp->m_growlock)) {
> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
> +		mutex_unlock(&mp->m_growlock);
> +	}

It looks like growfs returns an error if the lock not available. Did you
intend to perform the primary update and then skip the secondary updates
if the lock is not acquired?

Perhaps we should just trylock earlier and hold the lock across the
entire operation..? It's not like grow or relabel are frequent
operations.

Brian

> +out:
> +	mnt_drop_write_file(filp);
> +	return error;
> +}
> +
>  /*
>   * Note: some of the ioctl's return positive numbers as a
>   * byte count indicating success, such as readlink_by_handle.
> @@ -1834,6 +1913,10 @@ struct getfsmap_info {
>  	switch (cmd) {
>  	case FITRIM:
>  		return xfs_ioc_trim(mp, arg);
> +	case FS_IOC_GET_FSLABEL:
> +		return xfs_ioc_getlabel(mp, arg);
> +	case FS_IOC_SET_FSLABEL:
> +		return xfs_ioc_setlabel(filp, mp, arg);
>  	case XFS_IOC_ALLOCSP:
>  	case XFS_IOC_FREESP:
>  	case XFS_IOC_RESVSP:
> -- 
> 1.8.3.1
> 
> --
> 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

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Eric Sandeen]

On 5/1/18 9:18 AM, Brian Foster wrote:
> On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
>> The GET ioctl is trivial, just return the current label.
>>
>> The SET ioctl is more involved:
>> It transactionally modifies the superblock to write a new filesystem
>> label to the primary super.
>>
>> It then also checkpoints the log to disk so that the change lands in
>> block 0, invalidates any page cache that userspace might have previously
>> read, and updates all secondary superblocks as userspace relable does.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> nb: the growfs trylock is just cargo-culting; growfs also trylocks.  *shrug*
>>
>>  fs/xfs/xfs_ioctl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 89fb1eb..12c6711 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -20,6 +20,7 @@
>>  #include "xfs_shared.h"
>>  #include "xfs_format.h"
>>  #include "xfs_log_format.h"
>> +#include "xfs_log.h"
>>  #include "xfs_trans_resv.h"
>>  #include "xfs_mount.h"
>>  #include "xfs_inode.h"
>> @@ -1811,6 +1812,84 @@ struct getfsmap_info {
>>  	return error;
>>  }
>>  
>> +static int
>> +xfs_ioc_getlabel(
>> +	struct xfs_mount	*mp,
>> +	char			__user *label)
>> +{
>> +	struct xfs_sb		*sbp = &mp->m_sb;
>> +
>> +	/* Paranoia */
>> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
>> +
>> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
>> +		return -EFAULT;
>> +	return 0;
>> +}
>> +
>> +static int
>> +xfs_ioc_setlabel(
>> +	struct file		*filp,
>> +	struct xfs_mount	*mp,
>> +	char			__user *newlabel)
>> +{
>> +	struct address_space	*mapping;
>> +	struct xfs_sb		*sbp = &mp->m_sb;
>> +	char			label[FSLABEL_MAX];
>> +	size_t			len;
>> +	int			error;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
>> +		return -EFAULT;
>> +	/*
>> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
>> +	 * smaller, at 12 bytes.
>> +	 * NB: The on disk label doesn't need to be null terminated.
>> +	 */
>> +	len = strnlen(label, FSLABEL_MAX);
>> +	if (len > sizeof(sbp->sb_fname)) {
>> +		xfs_warn(mp, "label %s is too long, max %zu chars",
>> +			label, sizeof(sbp->sb_fname));
>> +		return -EINVAL;
>> +	}
>> +
>> +	error = mnt_want_write_file(filp);
>> +	if (error)
>> +		return error;
>> +
>> +	spin_lock(&mp->m_sb_lock);
>> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
>> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
>> +	spin_unlock(&mp->m_sb_lock);
>> +
>> +	error = xfs_sync_sb(mp, true);
>> +	if (error)
>> +		goto out;
> 
> Is there a reason this all has to be synchronous in the first place, as
> opposed to using xfs_log_sb() and committing a transaction for example?
> Is it because we essentially don't have a way to recover if we crash
> partway through? (Whatever the reason, a brief comment around why is
> useful for future reference).

Well, we want userspace to see it in sector 0 after the call returns,
I don't want it waiting around in the log.  That's why I was doing
sync-all-the-things-now.

>> +
>> +	/* checkpoint the log to update primary super in fs itself */
>> +	error = xfs_log_checkpoint(mp);
>> +	if (error)
>> +		goto out;
>> +
> 
> xfs_sync_sb() with wait == true marks the transaction sync, which does
> the log force part (but doesn't push the ail) based on the lsn of the
> commit. Also note that you should be able to push the AIL based on the
> lsn in the log item, which is set on AIL insertion, as opposed to doing
> a full AIL push as well.

It's kind of ridiculous, but I don't speak log very well.  I didn't see a
downside to just pushing it all, but can you offer a code snippet for what
you're proposing, if you think that'd be better?

>> +	/* invalidate any cached bdev page for userspace visibility */
>> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
>> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
>> +	if (error)
>> +		goto out;
> 
> A more detailed comment on why this is necessary would be helpful. :)

Why we need to invalidate cached pages on the block device?  I thought that
was clear from the comment, but I can add more words.

/*
 * The block device may have cached pages for the block device from previous
 * reads in userspace, and we want i.e. blkid to see the new info, so invalidate
 * any cached pages now.
 */

Like that?

>> +
>> +	/* update the backup superblocks like userspace does */
>> +	if (mutex_trylock(&mp->m_growlock)) {
>> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
>> +		mutex_unlock(&mp->m_growlock);
>> +	}
> 
> It looks like growfs returns an error if the lock not available. Did you
> intend to perform the primary update and then skip the secondary updates
> if the lock is not acquired?
> 
> Perhaps we should just trylock earlier and hold the lock across the
> entire operation..? It's not like grow or relabel are frequent
> operations.

yeah, I was on the fence about this.  Updating the secondaries with the new label
is not strictly required; repair doesn't care at all, and online repair considers
it a "preen" action.  TBH I was lazily unsure about growlock vs. m_sb_lock nesting
and didn't want to risk getting it wrong.

> Brian

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Brian Foster]

On Tue, May 01, 2018 at 09:27:30AM -0500, Eric Sandeen wrote:
> On 5/1/18 9:18 AM, Brian Foster wrote:
> > On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
> >> The GET ioctl is trivial, just return the current label.
> >>
> >> The SET ioctl is more involved:
> >> It transactionally modifies the superblock to write a new filesystem
> >> label to the primary super.
> >>
> >> It then also checkpoints the log to disk so that the change lands in
> >> block 0, invalidates any page cache that userspace might have previously
> >> read, and updates all secondary superblocks as userspace relable does.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> nb: the growfs trylock is just cargo-culting; growfs also trylocks.  *shrug*
> >>
> >>  fs/xfs/xfs_ioctl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 83 insertions(+)
> >>
> >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> >> index 89fb1eb..12c6711 100644
> >> --- a/fs/xfs/xfs_ioctl.c
> >> +++ b/fs/xfs/xfs_ioctl.c
> >> @@ -20,6 +20,7 @@
> >>  #include "xfs_shared.h"
> >>  #include "xfs_format.h"
> >>  #include "xfs_log_format.h"
> >> +#include "xfs_log.h"
> >>  #include "xfs_trans_resv.h"
> >>  #include "xfs_mount.h"
> >>  #include "xfs_inode.h"
> >> @@ -1811,6 +1812,84 @@ struct getfsmap_info {
> >>  	return error;
> >>  }
> >>  
> >> +static int
> >> +xfs_ioc_getlabel(
> >> +	struct xfs_mount	*mp,
> >> +	char			__user *label)
> >> +{
> >> +	struct xfs_sb		*sbp = &mp->m_sb;
> >> +
> >> +	/* Paranoia */
> >> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> >> +
> >> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> >> +		return -EFAULT;
> >> +	return 0;
> >> +}
> >> +
> >> +static int
> >> +xfs_ioc_setlabel(
> >> +	struct file		*filp,
> >> +	struct xfs_mount	*mp,
> >> +	char			__user *newlabel)
> >> +{
> >> +	struct address_space	*mapping;
> >> +	struct xfs_sb		*sbp = &mp->m_sb;
> >> +	char			label[FSLABEL_MAX];
> >> +	size_t			len;
> >> +	int			error;
> >> +
> >> +	if (!capable(CAP_SYS_ADMIN))
> >> +		return -EPERM;
> >> +
> >> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
> >> +		return -EFAULT;
> >> +	/*
> >> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
> >> +	 * smaller, at 12 bytes.
> >> +	 * NB: The on disk label doesn't need to be null terminated.
> >> +	 */
> >> +	len = strnlen(label, FSLABEL_MAX);
> >> +	if (len > sizeof(sbp->sb_fname)) {
> >> +		xfs_warn(mp, "label %s is too long, max %zu chars",
> >> +			label, sizeof(sbp->sb_fname));
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	error = mnt_want_write_file(filp);
> >> +	if (error)
> >> +		return error;
> >> +
> >> +	spin_lock(&mp->m_sb_lock);
> >> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> >> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> >> +	spin_unlock(&mp->m_sb_lock);
> >> +
> >> +	error = xfs_sync_sb(mp, true);
> >> +	if (error)
> >> +		goto out;
> > 
> > Is there a reason this all has to be synchronous in the first place, as
> > opposed to using xfs_log_sb() and committing a transaction for example?
> > Is it because we essentially don't have a way to recover if we crash
> > partway through? (Whatever the reason, a brief comment around why is
> > useful for future reference).
> 
> Well, we want userspace to see it in sector 0 after the call returns,
> I don't want it waiting around in the log.  That's why I was doing
> sync-all-the-things-now.
> 

Ok. It doesn't really seem like a problem given the operation. I was
just curious.

> >> +
> >> +	/* checkpoint the log to update primary super in fs itself */
> >> +	error = xfs_log_checkpoint(mp);
> >> +	if (error)
> >> +		goto out;
> >> +
> > 
> > xfs_sync_sb() with wait == true marks the transaction sync, which does
> > the log force part (but doesn't push the ail) based on the lsn of the
> > commit. Also note that you should be able to push the AIL based on the
> > lsn in the log item, which is set on AIL insertion, as opposed to doing
> > a full AIL push as well.
> 
> It's kind of ridiculous, but I don't speak log very well.  I didn't see a
> downside to just pushing it all, but can you offer a code snippet for what
> you're proposing, if you think that'd be better?
> 

It's just a bit of overkill. The log force is unnecessary. If you do end
up using xfs_sync_sb(), then I think calling xfs_ail_push() with the
->li_lsn from the superblock log buffer would be sufficient. But er,
that's async and I'm not sure we have anything atm that does an LSN
targeted sync ail push. Perhaps we could create one based on
xfs_ail_push_all_sync()?

> >> +	/* invalidate any cached bdev page for userspace visibility */
> >> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
> >> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
> >> +	if (error)
> >> +		goto out;
> > 
> > A more detailed comment on why this is necessary would be helpful. :)
> 
> Why we need to invalidate cached pages on the block device?  I thought that
> was clear from the comment, but I can add more words.
> 

I'm not really sure what "userspace visibility" means. Wouldn't
userspace just call the get label ioctl?

> /*
>  * The block device may have cached pages for the block device from previous
>  * reads in userspace, and we want i.e. blkid to see the new info, so invalidate
>  * any cached pages now.
>  */
> 
> Like that?
> 

I suppose. Does something actually break without this? Why do we care
about bdev inode cache invalidation here and not with any other
superblock modifications?

> >> +
> >> +	/* update the backup superblocks like userspace does */
> >> +	if (mutex_trylock(&mp->m_growlock)) {
> >> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
> >> +		mutex_unlock(&mp->m_growlock);
> >> +	}
> > 
> > It looks like growfs returns an error if the lock not available. Did you
> > intend to perform the primary update and then skip the secondary updates
> > if the lock is not acquired?
> > 
> > Perhaps we should just trylock earlier and hold the lock across the
> > entire operation..? It's not like grow or relabel are frequent
> > operations.
> 
> yeah, I was on the fence about this.  Updating the secondaries with the new label
> is not strictly required; repair doesn't care at all, and online repair considers
> it a "preen" action.  TBH I was lazily unsure about growlock vs. m_sb_lock nesting
> and didn't want to risk getting it wrong.
> 

I have no strong preference either way, but ISTM we should decide
whether to update secondary supers or not.

Brian

> > Brian

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Darrick J. Wong]

On Tue, May 01, 2018 at 10:42:38AM -0400, Brian Foster wrote:
> On Tue, May 01, 2018 at 09:27:30AM -0500, Eric Sandeen wrote:
> > On 5/1/18 9:18 AM, Brian Foster wrote:
> > > On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
> > >> The GET ioctl is trivial, just return the current label.
> > >>
> > >> The SET ioctl is more involved:
> > >> It transactionally modifies the superblock to write a new filesystem
> > >> label to the primary super.
> > >>
> > >> It then also checkpoints the log to disk so that the change lands in
> > >> block 0, invalidates any page cache that userspace might have previously
> > >> read, and updates all secondary superblocks as userspace relable does.
> > >>
> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > >> ---
> > >>
> > >> nb: the growfs trylock is just cargo-culting; growfs also trylocks.  *shrug*
> > >>
> > >>  fs/xfs/xfs_ioctl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 83 insertions(+)
> > >>
> > >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > >> index 89fb1eb..12c6711 100644
> > >> --- a/fs/xfs/xfs_ioctl.c
> > >> +++ b/fs/xfs/xfs_ioctl.c
> > >> @@ -20,6 +20,7 @@
> > >>  #include "xfs_shared.h"
> > >>  #include "xfs_format.h"
> > >>  #include "xfs_log_format.h"
> > >> +#include "xfs_log.h"
> > >>  #include "xfs_trans_resv.h"
> > >>  #include "xfs_mount.h"
> > >>  #include "xfs_inode.h"
> > >> @@ -1811,6 +1812,84 @@ struct getfsmap_info {
> > >>  	return error;
> > >>  }
> > >>  
> > >> +static int
> > >> +xfs_ioc_getlabel(
> > >> +	struct xfs_mount	*mp,
> > >> +	char			__user *label)
> > >> +{
> > >> +	struct xfs_sb		*sbp = &mp->m_sb;
> > >> +
> > >> +	/* Paranoia */
> > >> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> > >> +
> > >> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> > >> +		return -EFAULT;
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static int
> > >> +xfs_ioc_setlabel(
> > >> +	struct file		*filp,
> > >> +	struct xfs_mount	*mp,
> > >> +	char			__user *newlabel)
> > >> +{
> > >> +	struct address_space	*mapping;
> > >> +	struct xfs_sb		*sbp = &mp->m_sb;
> > >> +	char			label[FSLABEL_MAX];
> > >> +	size_t			len;
> > >> +	int			error;
> > >> +
> > >> +	if (!capable(CAP_SYS_ADMIN))
> > >> +		return -EPERM;
> > >> +
> > >> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
> > >> +		return -EFAULT;
> > >> +	/*
> > >> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
> > >> +	 * smaller, at 12 bytes.
> > >> +	 * NB: The on disk label doesn't need to be null terminated.
> > >> +	 */
> > >> +	len = strnlen(label, FSLABEL_MAX);
> > >> +	if (len > sizeof(sbp->sb_fname)) {
> > >> +		xfs_warn(mp, "label %s is too long, max %zu chars",
> > >> +			label, sizeof(sbp->sb_fname));
> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	error = mnt_want_write_file(filp);
> > >> +	if (error)
> > >> +		return error;
> > >> +
> > >> +	spin_lock(&mp->m_sb_lock);
> > >> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> > >> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> > >> +	spin_unlock(&mp->m_sb_lock);
> > >> +
> > >> +	error = xfs_sync_sb(mp, true);
> > >> +	if (error)
> > >> +		goto out;
> > > 
> > > Is there a reason this all has to be synchronous in the first place, as
> > > opposed to using xfs_log_sb() and committing a transaction for example?
> > > Is it because we essentially don't have a way to recover if we crash
> > > partway through? (Whatever the reason, a brief comment around why is
> > > useful for future reference).
> > 
> > Well, we want userspace to see it in sector 0 after the call returns,
> > I don't want it waiting around in the log.  That's why I was doing
> > sync-all-the-things-now.
> > 
> 
> Ok. It doesn't really seem like a problem given the operation. I was
> just curious.
> 
> > >> +
> > >> +	/* checkpoint the log to update primary super in fs itself */
> > >> +	error = xfs_log_checkpoint(mp);
> > >> +	if (error)
> > >> +		goto out;
> > >> +
> > > 
> > > xfs_sync_sb() with wait == true marks the transaction sync, which does
> > > the log force part (but doesn't push the ail) based on the lsn of the
> > > commit. Also note that you should be able to push the AIL based on the
> > > lsn in the log item, which is set on AIL insertion, as opposed to doing
> > > a full AIL push as well.
> > 
> > It's kind of ridiculous, but I don't speak log very well.  I didn't see a
> > downside to just pushing it all, but can you offer a code snippet for what
> > you're proposing, if you think that'd be better?
> > 
> 
> It's just a bit of overkill. The log force is unnecessary. If you do end
> up using xfs_sync_sb(), then I think calling xfs_ail_push() with the
> ->li_lsn from the superblock log buffer would be sufficient. But er,
> that's async and I'm not sure we have anything atm that does an LSN
> targeted sync ail push. Perhaps we could create one based on
> xfs_ail_push_all_sync()?

Sounds like a good fit here.

> > >> +	/* invalidate any cached bdev page for userspace visibility */
> > >> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
> > >> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
> > >> +	if (error)
> > >> +		goto out;
> > > 
> > > A more detailed comment on why this is necessary would be helpful. :)
> > 
> > Why we need to invalidate cached pages on the block device?  I thought that
> > was clear from the comment, but I can add more words.
> > 
> 
> I'm not really sure what "userspace visibility" means. Wouldn't
> userspace just call the get label ioctl?
> 
> > /*
> >  * The block device may have cached pages for the block device from previous
> >  * reads in userspace, and we want i.e. blkid to see the new info, so invalidate
> >  * any cached pages now.
> >  */
> > 
> > Like that?

/*
 * Userspace reads of the superblock may have created page cache for the
 * superblock behind our back, so invalidate those pages.
 */

Perhaps?

> > 
> 
> I suppose. Does something actually break without this? Why do we care
> about bdev inode cache invalidation here and not with any other
> superblock modifications?

blkid will return stale results if the page cache is stale.

> 
> > >> +
> > >> +	/* update the backup superblocks like userspace does */
> > >> +	if (mutex_trylock(&mp->m_growlock)) {
> > >> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
> > >> +		mutex_unlock(&mp->m_growlock);
> > >> +	}
> > > 
> > > It looks like growfs returns an error if the lock not available. Did you
> > > intend to perform the primary update and then skip the secondary updates
> > > if the lock is not acquired?
> > > 
> > > Perhaps we should just trylock earlier and hold the lock across the
> > > entire operation..? It's not like grow or relabel are frequent
> > > operations.
> > 
> > yeah, I was on the fence about this.  Updating the secondaries with the new label
> > is not strictly required; repair doesn't care at all, and online repair considers
> > it a "preen" action.  TBH I was lazily unsure about growlock vs. m_sb_lock nesting
> > and didn't want to risk getting it wrong.
> > 
> 
> I have no strong preference either way, but ISTM we should decide
> whether to update secondary supers or not.

Dave's 'tablise growfs' series converted all this to create a chain of
delwri buffers, which makes it tempting to refactor
xfs_sb_update_secondary().  However, iirc that discussion ended with
Dave saying he'd rework a few bits and resend... so either that needs to
happen, or we just push this in sooner and add refactoring the other
secondary super update (namely online repair) when the growfs work
lands.

--D

> 
> Brian
> 
> > > Brian
> --
> 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

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Eric Sandeen]

On 5/1/18 9:42 AM, Brian Foster wrote:
> On Tue, May 01, 2018 at 09:27:30AM -0500, Eric Sandeen wrote:


>>>> +	/* invalidate any cached bdev page for userspace visibility */
>>>> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
>>>> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
>>>> +	if (error)
>>>> +		goto out;
>>>
>>> A more detailed comment on why this is necessary would be helpful. :)
>>
>> Why we need to invalidate cached pages on the block device?  I thought that
>> was clear from the comment, but I can add more words.
>>
> 
> I'm not really sure what "userspace visibility" means. Wouldn't
> userspace just call the get label ioctl?

It could, but we also have blkid which reads the block device directly
today, even when mounted.  They need to co-exist, and blkid needs to see
the label change /on the block device/ after it's modified while mounted.

>> /*
>>  * The block device may have cached pages for the block device from previous
>>  * reads in userspace, and we want i.e. blkid to see the new info, so invalidate
>>  * any cached pages now.
>>  */
>>
>> Like that?
>>
> 
> I suppose. Does something actually break without this? Why do we care
> about bdev inode cache invalidation here and not with any other
> superblock modifications?

Because there's no other situation or utility that I'm aware of which reads
a mounted block device directly, and is gathering data which may now change
while mounted.

IOWSs, blkid reads things like label, uuid, fs type, etc, but none of those
can (could) change while mounted; now label /can/ change, and blkid must 
see it.

i.e. we don't want:

# blkid -s LABEL /dev/sdc1 
/dev/sdc1: LABEL="oldlabel" 

# xfs_io -c "label newlabel" /mnt/sdc1
label = newlabel

# blkid -s LABEL /dev/sdc1 
/dev/sdc1: LABEL="oldlabel" 

- we want to see the /new/ label.

>>>> +
>>>> +	/* update the backup superblocks like userspace does */
>>>> +	if (mutex_trylock(&mp->m_growlock)) {
>>>> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
>>>> +		mutex_unlock(&mp->m_growlock);
>>>> +	}
>>>
>>> It looks like growfs returns an error if the lock not available. Did you
>>> intend to perform the primary update and then skip the secondary updates
>>> if the lock is not acquired?
>>>
>>> Perhaps we should just trylock earlier and hold the lock across the
>>> entire operation..? It's not like grow or relabel are frequent
>>> operations.
>>
>> yeah, I was on the fence about this.  Updating the secondaries with the new label
>> is not strictly required; repair doesn't care at all, and online repair considers
>> it a "preen" action.  TBH I was lazily unsure about growlock vs. m_sb_lock nesting
>> and didn't want to risk getting it wrong.
>>
> 
> I have no strong preference either way, but ISTM we should decide
> whether to update secondary supers or not.

Ok.  I'll wait for the growfs lock to ensure we do the secondary updates, I guess.
Doing everything else and then returning -EBUSY for the secondaries would be
confusing IMHO.

-Eric

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Dave Chinner]

On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
> The GET ioctl is trivial, just return the current label.
> 
> The SET ioctl is more involved:
> It transactionally modifies the superblock to write a new filesystem
> label to the primary super.
> 
> It then also checkpoints the log to disk so that the change lands in
> block 0, invalidates any page cache that userspace might have previously
> read, and updates all secondary superblocks as userspace relable does.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Just to close the loop on what Eric and I just talked about on
IRC....

> +xfs_ioc_setlabel(
> +	struct file		*filp,
> +	struct xfs_mount	*mp,
> +	char			__user *newlabel)
> +{
> +	struct address_space	*mapping;
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +	char			label[FSLABEL_MAX];
> +	size_t			len;
> +	int			error;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
> +		return -EFAULT;
> +	/*
> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
> +	 * smaller, at 12 bytes.
> +	 * NB: The on disk label doesn't need to be null terminated.
> +	 */
> +	len = strnlen(label, FSLABEL_MAX);
> +	if (len > sizeof(sbp->sb_fname)) {
> +		xfs_warn(mp, "label %s is too long, max %zu chars",
> +			label, sizeof(sbp->sb_fname));
> +		return -EINVAL;
> +	}
> +
> +	error = mnt_want_write_file(filp);
> +	if (error)
> +		return error;
> +
> +	spin_lock(&mp->m_sb_lock);
> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> +	spin_unlock(&mp->m_sb_lock);
> +
> +	error = xfs_sync_sb(mp, true);
> +	if (error)
> +		goto out;

This is all good up to here. We want a synchronous transaction to
force the change to the log and *unpin the superblock buffer* at the
same time.

However, we want to also write the superblock buffer, and so we
really should have a function that does this directly: i.e.

	/* Force superblock label changes to disk */
	error = xfs_sync_sb_buf(mp);
	if (error)
		goto out;

which does:

xfs_sync_sb_buf()
{
	/* same transaction preamble and logging as xfs_sync_sb() */
	.....

	xfs_trans_bhold(tp, bp);
	xfs_trans_set_sync(tp);
	error = xfs_trans_commit(tp);
	if (error)
		goto out;

	/*
	 * write out the sb buffer to get the changes to disk
	 */
	error = xfs_bwrite(bp);
	xfs_buf_relse(bp);
	return error;
}


> +	/* checkpoint the log to update primary super in fs itself */
> +	error = xfs_log_checkpoint(mp);
> +	if (error)
> +		goto out;
> +
> +	/* invalidate any cached bdev page for userspace visibility */
> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
> +	if (error)
> +		goto out;

invalidate_bdev()? We don't use this block device
mapping, so just invalidating it completely is probably appropriate.
FWIW, ISTR that userspace points at a different address space
mapping and we can't invalidate that directly ourselves? Worth
checking...


> +	/* update the backup superblocks like userspace does */
> +	if (mutex_trylock(&mp->m_growlock)) {
> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
> +		mutex_unlock(&mp->m_growlock);
> +	}

Why not just block here?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/5] xfs: implement online get/set fs label

[Eric Sandeen]

On 5/1/18 5:11 PM, Dave Chinner wrote:
> On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
>> The GET ioctl is trivial, just return the current label.
>>
>> The SET ioctl is more involved:
>> It transactionally modifies the superblock to write a new filesystem
>> label to the primary super.
>>
>> It then also checkpoints the log to disk so that the change lands in
>> block 0, invalidates any page cache that userspace might have previously
>> read, and updates all secondary superblocks as userspace relable does.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
> 
> Just to close the loop on what Eric and I just talked about on
> IRC....

Gracias.

>> +xfs_ioc_setlabel(
>> +	struct file		*filp,
>> +	struct xfs_mount	*mp,
>> +	char			__user *newlabel)
>> +{
>> +	struct address_space	*mapping;
>> +	struct xfs_sb		*sbp = &mp->m_sb;
>> +	char			label[FSLABEL_MAX];
>> +	size_t			len;
>> +	int			error;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
>> +		return -EFAULT;
>> +	/*
>> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
>> +	 * smaller, at 12 bytes.
>> +	 * NB: The on disk label doesn't need to be null terminated.
>> +	 */
>> +	len = strnlen(label, FSLABEL_MAX);
>> +	if (len > sizeof(sbp->sb_fname)) {
>> +		xfs_warn(mp, "label %s is too long, max %zu chars",
>> +			label, sizeof(sbp->sb_fname));
>> +		return -EINVAL;
>> +	}
>> +
>> +	error = mnt_want_write_file(filp);
>> +	if (error)
>> +		return error;
>> +
>> +	spin_lock(&mp->m_sb_lock);
>> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
>> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
>> +	spin_unlock(&mp->m_sb_lock);
>> +
>> +	error = xfs_sync_sb(mp, true);
>> +	if (error)
>> +		goto out;
> 
> This is all good up to here. We want a synchronous transaction to
> force the change to the log and *unpin the superblock buffer* at the
> same time.
> 
> However, we want to also write the superblock buffer, and so we
> really should have a function that does this directly: i.e.
> 
> 	/* Force superblock label changes to disk */
> 	error = xfs_sync_sb_buf(mp);
> 	if (error)
> 		goto out;
> 
> which does:
> 
> xfs_sync_sb_buf()
> {
> 	/* same transaction preamble and logging as xfs_sync_sb() */
> 	.....
> 
> 	xfs_trans_bhold(tp, bp);
> 	xfs_trans_set_sync(tp);
> 	error = xfs_trans_commit(tp);
> 	if (error)
> 		goto out;
> 
> 	/*
> 	 * write out the sb buffer to get the changes to disk
> 	 */
> 	error = xfs_bwrite(bp);
> 	xfs_buf_relse(bp);
> 	return error;
> }

Thank you thank you thank you :)  That looks awesome, let me try this.

> 
>> +	/* checkpoint the log to update primary super in fs itself */
>> +	error = xfs_log_checkpoint(mp);
>> +	if (error)
>> +		goto out;
>> +
>> +	/* invalidate any cached bdev page for userspace visibility */
>> +	mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
>> +	error = invalidate_inode_pages2_range(mapping, 0, 0);
>> +	if (error)
>> +		goto out;
> 
> invalidate_bdev()? We don't use this block device
> mapping, so just invalidating it completely is probably appropriate.
> FWIW, ISTR that userspace points at a different address space
> mapping and we can't invalidate that directly ourselves? Worth
> checking...

I thought the same thing.  :)  But it doesn't seem to be the case; the
above really does work, and it really is the same address space as verified
by a couple printks.  ;)

I guess invalidating the whole device is fine, but is there any real reason
to?  Maybe we would like xfs_db to see the secondaries as well?  *Shrug*
invalidate_bdev() is easier to call, I'll try it.

> 
>> +	/* update the backup superblocks like userspace does */
>> +	if (mutex_trylock(&mp->m_growlock)) {
>> +		error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
>> +		mutex_unlock(&mp->m_growlock);
>> +	}
> 
> Why not just block here?

Yeah, I'll change it.

Thanks!
-Eric

[PATCH 4/5 V2] xfs: implement online get/set fs label

[Eric Sandeen]

The GET ioctl is trivial, just return the current label.

The SET ioctl is more involved:
It transactionally modifies the superblock to write a new filesystem
label to the primary super.

A new variant of xfs_sync_sb then writes the superblock buffer
immediately to disk so that the change is visible from userspace.

It then invalidates any page cache that userspace might have previously
read on the block device so that i.e. blkid can see the change
immediately, and updates all secondary superblocks as userspace relable
does.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: rework the force-sb-to-disk approach, invalidate the whole block
device, and block waiting for the growfs lock.  Also remove too-long-label
printk.

Thanks to dchinner for the xfs_sync_sb_buf suggestion & framework.

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index d9b94bd..54992e8 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -888,6 +888,37 @@ struct xfs_perag *
 	return xfs_trans_commit(tp);
 }
 
+/*
+ * Same behavior as xfs_sync_sb, except that it is always synchronous and it
+ * also writes the superblock buffer to disk sector 0 immediately.
+ */
+int
+xfs_sync_sb_buf(
+	struct xfs_mount	*mp)
+{
+	struct xfs_trans	*tp;
+	int			error;
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0,
+			XFS_TRANS_NO_WRITECOUNT, &tp);
+	if (error)
+		return error;
+
+	xfs_log_sb(tp);
+	xfs_trans_bhold(tp, mp->m_sb_bp);
+	xfs_trans_set_sync(tp);
+	error = xfs_trans_commit(tp);
+	if (error)
+		goto out;
+	/*
+	 * write out the sb buffer to get the changes to disk
+	 */
+	error = xfs_bwrite(mp->m_sb_bp);
+out:
+	xfs_buf_relse(mp->m_sb_bp);
+	return error;
+}
+
 int
 xfs_fs_geometry(
 	struct xfs_sb		*sbp,
diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index 63dcd2a..2268272 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -29,6 +29,7 @@ extern struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *, xfs_agnumber_t,
 
 extern void	xfs_log_sb(struct xfs_trans *tp);
 extern int	xfs_sync_sb(struct xfs_mount *mp, bool wait);
+extern int	xfs_sync_sb_buf(struct xfs_mount *mp);
 extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
 extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
 extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 89fb1eb..effb23a 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1811,6 +1811,72 @@ struct getfsmap_info {
 	return error;
 }
 
+static int
+xfs_ioc_getlabel(
+	struct xfs_mount	*mp,
+	char			__user *label)
+{
+	struct xfs_sb		*sbp = &mp->m_sb;
+
+	/* Paranoia */
+	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
+
+	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
+		return -EFAULT;
+	return 0;
+}
+
+static int
+xfs_ioc_setlabel(
+	struct file		*filp,
+	struct xfs_mount	*mp,
+	char			__user *newlabel)
+{
+	struct xfs_sb		*sbp = &mp->m_sb;
+	char			label[FSLABEL_MAX];
+	size_t			len;
+	int			error;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(label, newlabel, FSLABEL_MAX))
+		return -EFAULT;
+	/*
+	 * The generic ioctl allows up to FSLABEL_MAX chars, but XFS is much
+	 * smaller, at 12 bytes.
+	 * NB: The on disk label doesn't need to be null terminated.
+	 */
+	len = strnlen(label, FSLABEL_MAX);
+	if (len > sizeof(sbp->sb_fname))
+		return -EINVAL;
+
+	error = mnt_want_write_file(filp);
+	if (error)
+		return error;
+
+	spin_lock(&mp->m_sb_lock);
+	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
+	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
+	spin_unlock(&mp->m_sb_lock);
+
+	/* Log primary superblock label changes & force to disk. */
+	error = xfs_sync_sb_buf(mp);
+	if (error)
+		goto out;
+
+	/* Invalidate any cached bdev page for userspace visibility. */
+	invalidate_bdev(mp->m_ddev_targp->bt_bdev);
+
+	/* Update the backup superblocks like userspace does. */
+	mutex_lock(&mp->m_growlock);
+	error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
+	mutex_unlock(&mp->m_growlock);
+out:
+	mnt_drop_write_file(filp);
+	return error;
+}
+
 /*
  * Note: some of the ioctl's return positive numbers as a
  * byte count indicating success, such as readlink_by_handle.
@@ -1834,6 +1900,10 @@ struct getfsmap_info {
 	switch (cmd) {
 	case FITRIM:
 		return xfs_ioc_trim(mp, arg);
+	case FS_IOC_GET_FSLABEL:
+		return xfs_ioc_getlabel(mp, arg);
+	case FS_IOC_SET_FSLABEL:
+		return xfs_ioc_setlabel(filp, mp, arg);
 	case XFS_IOC_ALLOCSP:
 	case XFS_IOC_FREESP:
 	case XFS_IOC_RESVSP:

Re: [PATCH 4/5 V2] xfs: implement online get/set fs label

[Brian Foster]

On Tue, May 01, 2018 at 06:04:09PM -0500, Eric Sandeen wrote:
> The GET ioctl is trivial, just return the current label.
> 
> The SET ioctl is more involved:
> It transactionally modifies the superblock to write a new filesystem
> label to the primary super.
> 
> A new variant of xfs_sync_sb then writes the superblock buffer
> immediately to disk so that the change is visible from userspace.
> 
> It then invalidates any page cache that userspace might have previously
> read on the block device so that i.e. blkid can see the change
> immediately, and updates all secondary superblocks as userspace relable
> does.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: rework the force-sb-to-disk approach, invalidate the whole block
> device, and block waiting for the growfs lock.  Also remove too-long-label
> printk.
> 
> Thanks to dchinner for the xfs_sync_sb_buf suggestion & framework.
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index d9b94bd..54992e8 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -888,6 +888,37 @@ struct xfs_perag *
>  	return xfs_trans_commit(tp);
>  }
>  
> +/*
> + * Same behavior as xfs_sync_sb, except that it is always synchronous and it
> + * also writes the superblock buffer to disk sector 0 immediately.
> + */
> +int
> +xfs_sync_sb_buf(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0,
> +			XFS_TRANS_NO_WRITECOUNT, &tp);
> +	if (error)
> +		return error;
> +
> +	xfs_log_sb(tp);
> +	xfs_trans_bhold(tp, mp->m_sb_bp);
> +	xfs_trans_set_sync(tp);
> +	error = xfs_trans_commit(tp);
> +	if (error)
> +		goto out;
> +	/*
> +	 * write out the sb buffer to get the changes to disk
> +	 */
> +	error = xfs_bwrite(mp->m_sb_bp);

Ah, much more simple to just write out the buffer directly. Looks pretty
good to me once those comment updates are incorporated.. ;)

Brian

> +out:
> +	xfs_buf_relse(mp->m_sb_bp);
> +	return error;
> +}
> +
>  int
>  xfs_fs_geometry(
>  	struct xfs_sb		*sbp,
> diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> index 63dcd2a..2268272 100644
> --- a/fs/xfs/libxfs/xfs_sb.h
> +++ b/fs/xfs/libxfs/xfs_sb.h
> @@ -29,6 +29,7 @@ extern struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *, xfs_agnumber_t,
>  
>  extern void	xfs_log_sb(struct xfs_trans *tp);
>  extern int	xfs_sync_sb(struct xfs_mount *mp, bool wait);
> +extern int	xfs_sync_sb_buf(struct xfs_mount *mp);
>  extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
>  extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
>  extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 89fb1eb..effb23a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1811,6 +1811,72 @@ struct getfsmap_info {
>  	return error;
>  }
>  
> +static int
> +xfs_ioc_getlabel(
> +	struct xfs_mount	*mp,
> +	char			__user *label)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +
> +	/* Paranoia */
> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> +
> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int
> +xfs_ioc_setlabel(
> +	struct file		*filp,
> +	struct xfs_mount	*mp,
> +	char			__user *newlabel)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +	char			label[FSLABEL_MAX];
> +	size_t			len;
> +	int			error;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
> +		return -EFAULT;
> +	/*
> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but XFS is much
> +	 * smaller, at 12 bytes.
> +	 * NB: The on disk label doesn't need to be null terminated.
> +	 */
> +	len = strnlen(label, FSLABEL_MAX);
> +	if (len > sizeof(sbp->sb_fname))
> +		return -EINVAL;
> +
> +	error = mnt_want_write_file(filp);
> +	if (error)
> +		return error;
> +
> +	spin_lock(&mp->m_sb_lock);
> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> +	spin_unlock(&mp->m_sb_lock);
> +
> +	/* Log primary superblock label changes & force to disk. */
> +	error = xfs_sync_sb_buf(mp);
> +	if (error)
> +		goto out;
> +
> +	/* Invalidate any cached bdev page for userspace visibility. */
> +	invalidate_bdev(mp->m_ddev_targp->bt_bdev);
> +
> +	/* Update the backup superblocks like userspace does. */
> +	mutex_lock(&mp->m_growlock);
> +	error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
> +	mutex_unlock(&mp->m_growlock);
> +out:
> +	mnt_drop_write_file(filp);
> +	return error;
> +}
> +
>  /*
>   * Note: some of the ioctl's return positive numbers as a
>   * byte count indicating success, such as readlink_by_handle.
> @@ -1834,6 +1900,10 @@ struct getfsmap_info {
>  	switch (cmd) {
>  	case FITRIM:
>  		return xfs_ioc_trim(mp, arg);
> +	case FS_IOC_GET_FSLABEL:
> +		return xfs_ioc_getlabel(mp, arg);
> +	case FS_IOC_SET_FSLABEL:
> +		return xfs_ioc_setlabel(filp, mp, arg);
>  	case XFS_IOC_ALLOCSP:
>  	case XFS_IOC_FREESP:
>  	case XFS_IOC_RESVSP:
> --
> 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

Re: [PATCH 4/5 V2] xfs: implement online get/set fs label

[Darrick J. Wong]

On Tue, May 01, 2018 at 06:04:09PM -0500, Eric Sandeen wrote:
> The GET ioctl is trivial, just return the current label.
> 
> The SET ioctl is more involved:
> It transactionally modifies the superblock to write a new filesystem
> label to the primary super.
> 
> A new variant of xfs_sync_sb then writes the superblock buffer
> immediately to disk so that the change is visible from userspace.
> 
> It then invalidates any page cache that userspace might have previously
> read on the block device so that i.e. blkid can see the change
> immediately, and updates all secondary superblocks as userspace relable
> does.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: rework the force-sb-to-disk approach, invalidate the whole block
> device, and block waiting for the growfs lock.  Also remove too-long-label
> printk.
> 
> Thanks to dchinner for the xfs_sync_sb_buf suggestion & framework.
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index d9b94bd..54992e8 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -888,6 +888,37 @@ struct xfs_perag *
>  	return xfs_trans_commit(tp);
>  }
>  
> +/*
> + * Same behavior as xfs_sync_sb, except that it is always synchronous and it
> + * also writes the superblock buffer to disk sector 0 immediately.
> + */
> +int
> +xfs_sync_sb_buf(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0,
> +			XFS_TRANS_NO_WRITECOUNT, &tp);

I suppose this is a straight clone of xfs_sync_sb, but do we need
NO_WRITECOUNT here?  Will this get called while the fs is frozen?

> +	if (error)
> +		return error;
> +
> +	xfs_log_sb(tp);
> +	xfs_trans_bhold(tp, mp->m_sb_bp);
> +	xfs_trans_set_sync(tp);
> +	error = xfs_trans_commit(tp);
> +	if (error)
> +		goto out;
> +	/*
> +	 * write out the sb buffer to get the changes to disk
> +	 */
> +	error = xfs_bwrite(mp->m_sb_bp);
> +out:
> +	xfs_buf_relse(mp->m_sb_bp);
> +	return error;
> +}
> +
>  int
>  xfs_fs_geometry(
>  	struct xfs_sb		*sbp,
> diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> index 63dcd2a..2268272 100644
> --- a/fs/xfs/libxfs/xfs_sb.h
> +++ b/fs/xfs/libxfs/xfs_sb.h
> @@ -29,6 +29,7 @@ extern struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *, xfs_agnumber_t,
>  
>  extern void	xfs_log_sb(struct xfs_trans *tp);
>  extern int	xfs_sync_sb(struct xfs_mount *mp, bool wait);
> +extern int	xfs_sync_sb_buf(struct xfs_mount *mp);
>  extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
>  extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
>  extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 89fb1eb..effb23a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1811,6 +1811,72 @@ struct getfsmap_info {
>  	return error;
>  }
>  
> +static int
> +xfs_ioc_getlabel(
> +	struct xfs_mount	*mp,
> +	char			__user *label)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +
> +	/* Paranoia */
> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> +
> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))

Needs to ensure that a null is set at the end of the (userspace) buffer
just in case the label is "123456789012".

There's nothing in the documentation for this ioctl <cough> that says
the passed in buffer must already be zeroed.

> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int
> +xfs_ioc_setlabel(
> +	struct file		*filp,
> +	struct xfs_mount	*mp,
> +	char			__user *newlabel)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +	char			label[FSLABEL_MAX];
> +	size_t			len;
> +	int			error;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(label, newlabel, FSLABEL_MAX))
> +		return -EFAULT;
> +	/*
> +	 * The generic ioctl allows up to FSLABEL_MAX chars, but XFS is much
> +	 * smaller, at 12 bytes.
> +	 * NB: The on disk label doesn't need to be null terminated.
> +	 */
> +	len = strnlen(label, FSLABEL_MAX);
> +	if (len > sizeof(sbp->sb_fname))
> +		return -EINVAL;
> +
> +	error = mnt_want_write_file(filp);
> +	if (error)
> +		return error;
> +
> +	spin_lock(&mp->m_sb_lock);
> +	memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> +	strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> +	spin_unlock(&mp->m_sb_lock);
> +
> +	/* Log primary superblock label changes & force to disk. */
> +	error = xfs_sync_sb_buf(mp);
> +	if (error)
> +		goto out;
> +
> +	/* Invalidate any cached bdev page for userspace visibility. */
> +	invalidate_bdev(mp->m_ddev_targp->bt_bdev);
> +
> +	/* Update the backup superblocks like userspace does. */
> +	mutex_lock(&mp->m_growlock);
> +	error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
> +	mutex_unlock(&mp->m_growlock);
> +out:
> +	mnt_drop_write_file(filp);
> +	return error;

Looks ok enough to start testing.

--D

> +}
> +
>  /*
>   * Note: some of the ioctl's return positive numbers as a
>   * byte count indicating success, such as readlink_by_handle.
> @@ -1834,6 +1900,10 @@ struct getfsmap_info {
>  	switch (cmd) {
>  	case FITRIM:
>  		return xfs_ioc_trim(mp, arg);
> +	case FS_IOC_GET_FSLABEL:
> +		return xfs_ioc_getlabel(mp, arg);
> +	case FS_IOC_SET_FSLABEL:
> +		return xfs_ioc_setlabel(filp, mp, arg);
>  	case XFS_IOC_ALLOCSP:
>  	case XFS_IOC_FREESP:
>  	case XFS_IOC_RESVSP:
> --
> 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

Re: [PATCH 4/5 V2] xfs: implement online get/set fs label

[Eric Sandeen]

On 5/2/18 9:24 AM, Darrick J. Wong wrote:
> On Tue, May 01, 2018 at 06:04:09PM -0500, Eric Sandeen wrote:
>> The GET ioctl is trivial, just return the current label.
>>
>> The SET ioctl is more involved:
>> It transactionally modifies the superblock to write a new filesystem
>> label to the primary super.
>>
>> A new variant of xfs_sync_sb then writes the superblock buffer
>> immediately to disk so that the change is visible from userspace.
>>
>> It then invalidates any page cache that userspace might have previously
>> read on the block device so that i.e. blkid can see the change
>> immediately, and updates all secondary superblocks as userspace relable
>> does.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: rework the force-sb-to-disk approach, invalidate the whole block
>> device, and block waiting for the growfs lock.  Also remove too-long-label
>> printk.
>>
>> Thanks to dchinner for the xfs_sync_sb_buf suggestion & framework.
>>
>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>> index d9b94bd..54992e8 100644
>> --- a/fs/xfs/libxfs/xfs_sb.c
>> +++ b/fs/xfs/libxfs/xfs_sb.c
>> @@ -888,6 +888,37 @@ struct xfs_perag *
>>  	return xfs_trans_commit(tp);
>>  }
>>  
>> +/*
>> + * Same behavior as xfs_sync_sb, except that it is always synchronous and it
>> + * also writes the superblock buffer to disk sector 0 immediately.
>> + */
>> +int
>> +xfs_sync_sb_buf(
>> +	struct xfs_mount	*mp)
>> +{
>> +	struct xfs_trans	*tp;
>> +	int			error;
>> +
>> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0,
>> +			XFS_TRANS_NO_WRITECOUNT, &tp);
> 
> I suppose this is a straight clone of xfs_sync_sb, but do we need
> NO_WRITECOUNT here?  Will this get called while the fs is frozen?

No, I should remove that, thanks.  I might see how ugly it'd be to just
convert xfs_sync_sb() to take flags for wait, no_writecount, and flush_sb
or something. 
...
 
>> +static int
>> +xfs_ioc_getlabel(
>> +	struct xfs_mount	*mp,
>> +	char			__user *label)
>> +{
>> +	struct xfs_sb		*sbp = &mp->m_sb;
>> +
>> +	/* Paranoia */
>> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
>> +
>> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> 
> Needs to ensure that a null is set at the end of the (userspace) buffer
> just in case the label is "123456789012".

Oh, yup!

> There's nothing in the documentation for this ioctl <cough> that says
> the passed in buffer must already be zeroed.

where /do/ we document ioctls like this, anyway?

Documentation/ioctl/* I guess, though of course we don't.

Thanks,
-Eric

Re: [PATCH 4/5 V2] xfs: implement online get/set fs label

[Darrick J. Wong]

On Wed, May 02, 2018 at 09:28:35AM -0500, Eric Sandeen wrote:
> On 5/2/18 9:24 AM, Darrick J. Wong wrote:
> > On Tue, May 01, 2018 at 06:04:09PM -0500, Eric Sandeen wrote:
> >> The GET ioctl is trivial, just return the current label.
> >>
> >> The SET ioctl is more involved:
> >> It transactionally modifies the superblock to write a new filesystem
> >> label to the primary super.
> >>
> >> A new variant of xfs_sync_sb then writes the superblock buffer
> >> immediately to disk so that the change is visible from userspace.
> >>
> >> It then invalidates any page cache that userspace might have previously
> >> read on the block device so that i.e. blkid can see the change
> >> immediately, and updates all secondary superblocks as userspace relable
> >> does.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> V2: rework the force-sb-to-disk approach, invalidate the whole block
> >> device, and block waiting for the growfs lock.  Also remove too-long-label
> >> printk.
> >>
> >> Thanks to dchinner for the xfs_sync_sb_buf suggestion & framework.
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> >> index d9b94bd..54992e8 100644
> >> --- a/fs/xfs/libxfs/xfs_sb.c
> >> +++ b/fs/xfs/libxfs/xfs_sb.c
> >> @@ -888,6 +888,37 @@ struct xfs_perag *
> >>  	return xfs_trans_commit(tp);
> >>  }
> >>  
> >> +/*
> >> + * Same behavior as xfs_sync_sb, except that it is always synchronous and it
> >> + * also writes the superblock buffer to disk sector 0 immediately.
> >> + */
> >> +int
> >> +xfs_sync_sb_buf(
> >> +	struct xfs_mount	*mp)
> >> +{
> >> +	struct xfs_trans	*tp;
> >> +	int			error;
> >> +
> >> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0,
> >> +			XFS_TRANS_NO_WRITECOUNT, &tp);
> > 
> > I suppose this is a straight clone of xfs_sync_sb, but do we need
> > NO_WRITECOUNT here?  Will this get called while the fs is frozen?
> 
> No, I should remove that, thanks.  I might see how ugly it'd be to just
> convert xfs_sync_sb() to take flags for wait, no_writecount, and flush_sb
> or something. 
> ...
>  
> >> +static int
> >> +xfs_ioc_getlabel(
> >> +	struct xfs_mount	*mp,
> >> +	char			__user *label)
> >> +{
> >> +	struct xfs_sb		*sbp = &mp->m_sb;
> >> +
> >> +	/* Paranoia */
> >> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> >> +
> >> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> > 
> > Needs to ensure that a null is set at the end of the (userspace) buffer
> > just in case the label is "123456789012".
> 
> Oh, yup!
> 
> > There's nothing in the documentation for this ioctl <cough> that says
> > the passed in buffer must already be zeroed.
> 
> where /do/ we document ioctls like this, anyway?
> 
> Documentation/ioctl/* I guess, though of course we don't.

In the man-pages project, of course:
https://www.kernel.org/doc/man-pages/contributing.html

See example of previous efforts:
http://man7.org/linux/man-pages/man2/ioctl_getfsmap.2.html

--D

> 
> Thanks,
> -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

Re: [PATCH 4/5 V2] xfs: implement online get/set fs label

[Dave Chinner]

On Wed, May 02, 2018 at 09:28:35AM -0500, Eric Sandeen wrote:
> On 5/2/18 9:24 AM, Darrick J. Wong wrote:
> > On Tue, May 01, 2018 at 06:04:09PM -0500, Eric Sandeen wrote:
> >> The GET ioctl is trivial, just return the current label.
> >> +static int
> >> +xfs_ioc_getlabel(
> >> +	struct xfs_mount	*mp,
> >> +	char			__user *label)
> >> +{
> >> +	struct xfs_sb		*sbp = &mp->m_sb;
> >> +
> >> +	/* Paranoia */
> >> +	BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
> >> +
> >> +	if (copy_to_user(label, sbp->sb_fname, sizeof(sbp->sb_fname)))
> > 
> > Needs to ensure that a null is set at the end of the (userspace) buffer
> > just in case the label is "123456789012".
> 
> Oh, yup!
> 
> > There's nothing in the documentation for this ioctl <cough> that says
> > the passed in buffer must already be zeroed.
> 
> where /do/ we document ioctls like this, anyway?

man pages. You should really cc linux-api@vger.kernel.org on any
patch that introduces/hoists a new VFS ioctl, as well as the man
page patch to document it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 5/5] xfs_io: add label command

[Eric Sandeen]

This adds a get/set label command to xfs_io.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/io/Makefile b/io/Makefile
index 88e4751..b40156d 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -10,9 +10,9 @@ LSRCFILES = xfs_bmap.sh xfs_freeze.sh xfs_mkfile.sh
 HFILES = init.h io.h
 CFILES = init.c \
 	attr.c bmap.c cowextsize.c encrypt.c file.c freeze.c fsync.c \
-	getrusage.c imap.c link.c mmap.c open.c parent.c pread.c prealloc.c \
-	pwrite.c reflink.c scrub.c seek.c shutdown.c stat.c swapext.c sync.c \
-	truncate.c utimes.c
+	getrusage.c imap.c label.c link.c mmap.c open.c parent.c pread.c \
+	prealloc.c pwrite.c reflink.c scrub.c seek.c shutdown.c stat.c \
+	swapext.c sync.c truncate.c utimes.c
 
 LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG) $(LIBPTHREAD)
 LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG)
diff --git a/io/init.c b/io/init.c
index 0336c96..612962e 100644
--- a/io/init.c
+++ b/io/init.c
@@ -72,6 +72,7 @@ init_commands(void)
 	help_init();
 	imap_init();
 	inject_init();
+	label_init();
 	log_writes_init();
 	madvise_init();
 	mincore_init();
diff --git a/io/io.h b/io/io.h
index a267636..7f8197c 100644
--- a/io/io.h
+++ b/io/io.h
@@ -109,6 +109,7 @@ extern void		getrusage_init(void);
 extern void		help_init(void);
 extern void		imap_init(void);
 extern void		inject_init(void);
+extern void		label_init(void);
 extern void		mmap_init(void);
 extern void		open_init(void);
 extern void		parent_init(void);
diff --git a/io/label.c b/io/label.c
new file mode 100644
index 0000000..acab67f
--- /dev/null
+++ b/io/label.c
@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2018 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include "platform_defs.h"
+#include "libxfs.h"
+#include "path.h"
+#include "command.h"
+#include "init.h"
+#include "io.h"
+
+#ifndef FS_IOC_GET_FSLABEL
+/* Max chars for the interface; fs limits may differ */
+#define FSLABEL_MAX 256
+#define FS_IOC_GET_FSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
+#define FS_IOC_SET_FSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
+#endif
+
+static cmdinfo_t label_cmd;
+
+static int
+label_f(
+	int		argc,
+	char		**argv)
+{
+	int		error;
+	char		label[FSLABEL_MAX];
+
+	if (argc == 1) {
+		memset(&label, 0, sizeof(label));
+		error = ioctl(file->fd, FS_IOC_GET_FSLABEL, &label);
+	} else {
+		strncpy(label, argv[1], sizeof(label));
+		error = ioctl(file->fd, FS_IOC_SET_FSLABEL, &label);
+	}
+
+	if (error)
+		perror("label");
+	else
+		printf("label = \"%s\"\n", label);
+
+	return error;
+}
+
+void
+label_init(void)
+{
+	label_cmd.name = "label";
+	label_cmd.cfunc = label_f;
+	label_cmd.argmin = 0;
+	label_cmd.argmax = 1;
+	label_cmd.args = _("[label]");
+	label_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	label_cmd.oneline =
+		_("query or set the filesystem label while mounted");
+
+	add_command(&label_cmd);
+}
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index c3ab532..52b983d 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -1175,6 +1175,16 @@ This is intended to be equivalent to the shell command:
 See the
 .B log_writes
 command.
+.TP
+.BI "label" " " [ " label " ]
+On  filesystems  that  support  online label manipulation, set or get the
+filesystem label.  With no options, get the current filesystem label.
+With one option, set the filesystem label to
+.IR label .
+If the label is longer than the filesystem will accept,
+.B xfs_io
+will print an error message.  XFS filesystem labels can be at most 12
+characters long.
 .SH SEE ALSO
 .BR mkfs.xfs (8),
 .BR xfsctl (3),

Re: [PATCH 5/5] xfs_io: add label command

[Darrick J. Wong]

On Mon, Apr 30, 2018 at 10:48:54AM -0500, Eric Sandeen wrote:
> This adds a get/set label command to xfs_io.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/io/Makefile b/io/Makefile
> index 88e4751..b40156d 100644
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -10,9 +10,9 @@ LSRCFILES = xfs_bmap.sh xfs_freeze.sh xfs_mkfile.sh
>  HFILES = init.h io.h
>  CFILES = init.c \
>  	attr.c bmap.c cowextsize.c encrypt.c file.c freeze.c fsync.c \
> -	getrusage.c imap.c link.c mmap.c open.c parent.c pread.c prealloc.c \
> -	pwrite.c reflink.c scrub.c seek.c shutdown.c stat.c swapext.c sync.c \
> -	truncate.c utimes.c
> +	getrusage.c imap.c label.c link.c mmap.c open.c parent.c pread.c \
> +	prealloc.c pwrite.c reflink.c scrub.c seek.c shutdown.c stat.c \
> +	swapext.c sync.c truncate.c utimes.c
>  
>  LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG) $(LIBPTHREAD)
>  LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG)
> diff --git a/io/init.c b/io/init.c
> index 0336c96..612962e 100644
> --- a/io/init.c
> +++ b/io/init.c
> @@ -72,6 +72,7 @@ init_commands(void)
>  	help_init();
>  	imap_init();
>  	inject_init();
> +	label_init();
>  	log_writes_init();
>  	madvise_init();
>  	mincore_init();
> diff --git a/io/io.h b/io/io.h
> index a267636..7f8197c 100644
> --- a/io/io.h
> +++ b/io/io.h
> @@ -109,6 +109,7 @@ extern void		getrusage_init(void);
>  extern void		help_init(void);
>  extern void		imap_init(void);
>  extern void		inject_init(void);
> +extern void		label_init(void);
>  extern void		mmap_init(void);
>  extern void		open_init(void);
>  extern void		parent_init(void);
> diff --git a/io/label.c b/io/label.c
> new file mode 100644
> index 0000000..acab67f
> --- /dev/null
> +++ b/io/label.c
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright (c) 2018 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include <sys/ioctl.h>
> +#include <sys/mount.h>
> +#include "platform_defs.h"
> +#include "libxfs.h"
> +#include "path.h"
> +#include "command.h"
> +#include "init.h"
> +#include "io.h"
> +
> +#ifndef FS_IOC_GET_FSLABEL
> +/* Max chars for the interface; fs limits may differ */
> +#define FSLABEL_MAX 256
> +#define FS_IOC_GET_FSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
> +#define FS_IOC_SET_FSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> +#endif
> +
> +static cmdinfo_t label_cmd;
> +
> +static int
> +label_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	int		error;
> +	char		label[FSLABEL_MAX];
> +
> +	if (argc == 1) {
> +		memset(&label, 0, sizeof(label));
> +		error = ioctl(file->fd, FS_IOC_GET_FSLABEL, &label);
> +	} else {
> +		strncpy(label, argv[1], sizeof(label));
> +		error = ioctl(file->fd, FS_IOC_SET_FSLABEL, &label);
> +	}
> +
> +	if (error)
> +		perror("label");
> +	else
> +		printf("label = \"%s\"\n", label);
> +
> +	return error;
> +}
> +
> +void
> +label_init(void)
> +{
> +	label_cmd.name = "label";
> +	label_cmd.cfunc = label_f;
> +	label_cmd.argmin = 0;
> +	label_cmd.argmax = 1;
> +	label_cmd.args = _("[label]");
> +	label_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	label_cmd.oneline =
> +		_("query or set the filesystem label while mounted");
> +
> +	add_command(&label_cmd);
> +}
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index c3ab532..52b983d 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -1175,6 +1175,16 @@ This is intended to be equivalent to the shell command:
>  See the
>  .B log_writes
>  command.
> +.TP
> +.BI "label" " " [ " label " ]
> +On  filesystems  that  support  online label manipulation, set or get the
> +filesystem label.  With no options, get the current filesystem label.
> +With one option, set the filesystem label to
> +.IR label .
> +If the label is longer than the filesystem will accept,
> +.B xfs_io
> +will print an error message.  XFS filesystem labels can be at most 12
> +characters long.

Needs proper ioctl_[gs]etlabel manpage documenting the semantics of this
new xfs ioctl, though that should be sent to mkerrisk/linux-api so we
don't have to maintain it. :)

--D

>  .SH SEE ALSO
>  .BR mkfs.xfs (8),
>  .BR xfsctl (3),
> 
> --
> 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

[PATCH V2 0/5] xfs: online label

[Eric Sandeen]

resend of the whole series; broke out btrfs changes from vfs change
(can send btrfs changes (much) later), made my own backup super update
function to just avoid collision with dchinner, and changed the max length
handling a bit so we don't have 256-byte arrays on the stack.

xfs list only for now 'cause I probably did something wrong in xfs land
and don't want to subject everyone else to it.  ;)

Thanks,
-Eric

[PATCH 5/5] xfs_io: add label command

[Eric Sandeen]

This adds a get/set label command to xfs_io.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/io/Makefile b/io/Makefile
index 88e4751..b40156d 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -10,9 +10,9 @@ LSRCFILES = xfs_bmap.sh xfs_freeze.sh xfs_mkfile.sh
 HFILES = init.h io.h
 CFILES = init.c \
 	attr.c bmap.c cowextsize.c encrypt.c file.c freeze.c fsync.c \
-	getrusage.c imap.c link.c mmap.c open.c parent.c pread.c prealloc.c \
-	pwrite.c reflink.c scrub.c seek.c shutdown.c stat.c swapext.c sync.c \
-	truncate.c utimes.c
+	getrusage.c imap.c label.c link.c mmap.c open.c parent.c pread.c \
+	prealloc.c pwrite.c reflink.c scrub.c seek.c shutdown.c stat.c \
+	swapext.c sync.c truncate.c utimes.c
 
 LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG) $(LIBPTHREAD)
 LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG)
diff --git a/io/init.c b/io/init.c
index 0336c96..612962e 100644
--- a/io/init.c
+++ b/io/init.c
@@ -72,6 +72,7 @@ init_commands(void)
 	help_init();
 	imap_init();
 	inject_init();
+	label_init();
 	log_writes_init();
 	madvise_init();
 	mincore_init();
diff --git a/io/io.h b/io/io.h
index a267636..7f8197c 100644
--- a/io/io.h
+++ b/io/io.h
@@ -109,6 +109,7 @@ extern void		getrusage_init(void);
 extern void		help_init(void);
 extern void		imap_init(void);
 extern void		inject_init(void);
+extern void		label_init(void);
 extern void		mmap_init(void);
 extern void		open_init(void);
 extern void		parent_init(void);
diff --git a/io/label.c b/io/label.c
new file mode 100644
index 0000000..acab67f
--- /dev/null
+++ b/io/label.c
@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2018 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include "platform_defs.h"
+#include "libxfs.h"
+#include "path.h"
+#include "command.h"
+#include "init.h"
+#include "io.h"
+
+#ifndef FS_IOC_GET_FSLABEL
+/* Max chars for the interface; fs limits may differ */
+#define FSLABEL_MAX 256
+#define FS_IOC_GET_FSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
+#define FS_IOC_SET_FSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
+#endif
+
+static cmdinfo_t label_cmd;
+
+static int
+label_f(
+	int		argc,
+	char		**argv)
+{
+	int		error;
+	char		label[FSLABEL_MAX];
+
+	if (argc == 1) {
+		memset(&label, 0, sizeof(label));
+		error = ioctl(file->fd, FS_IOC_GET_FSLABEL, &label);
+	} else {
+		strncpy(label, argv[1], sizeof(label));
+		error = ioctl(file->fd, FS_IOC_SET_FSLABEL, &label);
+	}
+
+	if (error)
+		perror("label");
+	else
+		printf("label = \"%s\"\n", label);
+
+	return error;
+}
+
+void
+label_init(void)
+{
+	label_cmd.name = "label";
+	label_cmd.cfunc = label_f;
+	label_cmd.argmin = 0;
+	label_cmd.argmax = 1;
+	label_cmd.args = _("[label]");
+	label_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	label_cmd.oneline =
+		_("query or set the filesystem label while mounted");
+
+	add_command(&label_cmd);
+}
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index c3ab532..52b983d 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -1175,6 +1175,16 @@ This is intended to be equivalent to the shell command:
 See the
 .B log_writes
 command.
+.TP
+.BI "label" " " [ " label " ]
+On  filesystems  that  support  online label manipulation, set or get the
+filesystem label.  With no options, get the current filesystem label.
+With one option, set the filesystem label to
+.IR label .
+If the label is longer than the filesystem will accept,
+.B xfs_io
+will print an error message.  XFS filesystem labels can be at most 12
+characters long.
 .SH SEE ALSO
 .BR mkfs.xfs (8),
 .BR xfsctl (3),

--
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

Re: [PATCH 5/5] xfs_io: add label command

[Dave Chinner]

On Mon, May 14, 2018 at 12:43:20PM -0500, Eric Sandeen wrote:
> This adds a get/set label command to xfs_io.
....
> +static int
> +label_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	int		error;
> +	char		label[FSLABEL_MAX];
> +
> +	if (argc == 1) {
> +		memset(&label, 0, sizeof(label));
> +		error = ioctl(file->fd, FS_IOC_GET_FSLABEL, &label);
> +	} else {
> +		strncpy(label, argv[1], sizeof(label));
> +		error = ioctl(file->fd, FS_IOC_SET_FSLABEL, &label);
> +	}
> +
> +	if (error)
> +		perror("label");

Need to set exitcode = 1 here so that xfs_io fails with a non-zero
exit status....

> +	else
> +		printf("label = \"%s\"\n", label);
> +
> +	return error;

Because this isn't the exit status - this is the "continue
processing the next command" return value. i.e. return 0 if we want
to continue, non-zero if we want to abort further CLI processing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs_io: add label command

[Eric Sandeen]

On 5/14/18 11:32 PM, Dave Chinner wrote:
> On Mon, May 14, 2018 at 12:43:20PM -0500, Eric Sandeen wrote:
>> This adds a get/set label command to xfs_io.
> ....
>> +static int
>> +label_f(
>> +	int		argc,
>> +	char		**argv)
>> +{
>> +	int		error;
>> +	char		label[FSLABEL_MAX];
>> +
>> +	if (argc == 1) {
>> +		memset(&label, 0, sizeof(label));
>> +		error = ioctl(file->fd, FS_IOC_GET_FSLABEL, &label);
>> +	} else {
>> +		strncpy(label, argv[1], sizeof(label));
>> +		error = ioctl(file->fd, FS_IOC_SET_FSLABEL, &label);
>> +	}
>> +
>> +	if (error)
>> +		perror("label");
> 
> Need to set exitcode = 1 here so that xfs_io fails with a non-zero
> exit status....
> 
>> +	else
>> +		printf("label = \"%s\"\n", label);
>> +
>> +	return error;
> 
> Because this isn't the exit status - this is the "continue
> processing the next command" return value. i.e. return 0 if we want
> to continue, non-zero if we want to abort further CLI processing....

Hm, ok.  Today we only set exitcode for bmap, fiemap, freeze/thaw, and
imap.  It's really consistently used at all, and needs an audit I guess.

Maybe we should add an io_error() which sets it automatically, or something.

Anyway, I can add it here.

-Eric

Re: [PATCH 5/5] xfs_io: add label command

[Dave Chinner]

On Tue, May 15, 2018 at 10:06:53AM -0500, Eric Sandeen wrote:
> 
> 
> On 5/14/18 11:32 PM, Dave Chinner wrote:
> >On Mon, May 14, 2018 at 12:43:20PM -0500, Eric Sandeen wrote:
> >>This adds a get/set label command to xfs_io.
> >....
> >>+static int
> >>+label_f(
> >>+	int		argc,
> >>+	char		**argv)
> >>+{
> >>+	int		error;
> >>+	char		label[FSLABEL_MAX];
> >>+
> >>+	if (argc == 1) {
> >>+		memset(&label, 0, sizeof(label));
> >>+		error = ioctl(file->fd, FS_IOC_GET_FSLABEL, &label);
> >>+	} else {
> >>+		strncpy(label, argv[1], sizeof(label));
> >>+		error = ioctl(file->fd, FS_IOC_SET_FSLABEL, &label);
> >>+	}
> >>+
> >>+	if (error)
> >>+		perror("label");
> >
> >Need to set exitcode = 1 here so that xfs_io fails with a non-zero
> >exit status....
> >
> >>+	else
> >>+		printf("label = \"%s\"\n", label);
> >>+
> >>+	return error;
> >
> >Because this isn't the exit status - this is the "continue
> >processing the next command" return value. i.e. return 0 if we want
> >to continue, non-zero if we want to abort further CLI processing....
> 
> Hm, ok.  Today we only set exitcode for bmap, fiemap, freeze/thaw, and
> imap.  It's really consistently used at all, and needs an audit I guess.

You mean the audit I did 6 months ago?

https://www.spinics.net/lists/linux-xfs/msg13605.html

(which is why I know what the exitcode rules are. :)

The patch is still sitting around somewhere here....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com