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

[Hou Tao <houtao1@huawei.com>]

Hi Dave,

On 2017/9/1 10:06, Dave Chinner wrote:
> 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.
Maybe you miss the following "uuid_is_null(uuid)" check in xfs_uuid_mount() ?

xfs_uuid_mount(
    struct xfs_mount    *mp)
{

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

    if (uuid_is_null(uuid)) {
        xfs_warn(mp, "Filesystem has null UUID - can't mount");
        return -EINVAL;
    }

And we can clear the uuid of a XFS filesystem, and mount it with "nouuid" option successfully.
$ xfs_admin -U nil /dev/vda
$ mount -t xfs -o nouuid /dev/vda /tmp/vda

So I still think the null check in xfs_fs_uevent is needed.

Regards,

Tao