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

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

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