Re: [PATCH v2] xfs: add online uevent for mount operation

[Dave Chinner <david@fromorbit.com>]

On Fri, Sep 01, 2017 at 09:26:08AM +0800, Hou Tao wrote:
> Hi,
> 
> On 2017/9/1 7:34, Dave Chinner wrote:
> >>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> >>> index 3a3812b4..6f8351c 100644
> >>> --- a/fs/xfs/xfs_super.c
> >>> +++ b/fs/xfs/xfs_super.c
> >>> @@ -70,6 +70,11 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
> >>>  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
> >>>  #endif
> >>>  
> >>> +enum {
> >>> +	XFS_UEVENT_ENV_CNT = 2,
> >>
> >> XFS_UEVENT_MAX_ENV_COUNT ?
> > 
> > Why 2 when there's only one environment string passed?
> Will fix them, I take the last NULL pointer into account, and
> Yes, "XFS_UEVENT_MAX_ENV_COUNT" is a better name.
> 
> >>> +	XFS_UEVENT_UUID_LEN = UUID_STRING_LEN + 6,
> > 
> > And the magic number needs a comment.
> > 
> > Actually, I think it needs more than this - it's tightly bound to
> > the implementation in xfs_fs_uevent(), so this enum should be
> > defined there, not as a global all this distance away.....
> OK, I will move it into xfs_fs_uevent().
> 
> >>> +};
> >>> +
> >>>  /*
> >>>   * Table driven mount option parser.
> >>>   */
> >>> @@ -1530,6 +1535,28 @@ xfs_destroy_percpu_counters(
> >>>  	percpu_counter_destroy(&mp->m_fdblocks);
> >>>  }
> >>>  
> >>> +static void
> >>> +xfs_fs_uevent(
> >>> +	struct xfs_mount	*mp,
> >>> +	enum kobject_action	action)
> >>> +{
> >>> +	int err;
> >>> +	char *envp[XFS_UEVENT_ENV_CNT];
> >>> +	int i = 0;
> > 
> > Indent the variables to match the function declaration.
> I will fix them. I didn't event notice the indentations of these variables before.
> 
> >>> +
> >>> +	if (!uuid_is_null(&mp->m_super->s_uuid)) {
> > 
> > This will never be false. XFS filesystems should always have a valid
> > UUID.
> A null uuid is possible if we use "nouuid" to mount a XFS filesystem, so
> the check is still needed.

The "nouuid" mount option means "don't check if there is already a
filesystem ialready mounted with the same uuid as the one we are
mounting". It does not mean the filesystem does not have a UUID.

Indeed, in xfs_uuid_mount():

xfs_uuid_mount(
        struct xfs_mount        *mp)
{
        uuid_t                  *uuid = &mp->m_sb.sb_uuid;
        int                     hole, i;

        /* Publish UUID in struct super_block */
        uuid_copy(&mp->m_super->s_uuid, uuid);

        if (mp->m_flags & XFS_MOUNT_NOUUID)
                return 0;

We copy the filesystem's uuid into the VFS superblock before we
check the nouuid mount option flag. Hence a mounted XFS filesystem
always has a valid UUID in the superblock s_uuid field.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com