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

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

[Hou Tao]

It will be useful if there is a corresponding online uevent after
a XFS filesystem has been mounted. A typical usage of the uevent
is setting the error configuration for a specific XFS filesystem
or all XFS filesystems by using udevd.

The following is an example of udevd rule which will shutdown
any XFS filesystem after the filesystem gets any IO error:

    ACTION=="online", SUBSYSTEM=="xfs", DEVPATH=="/fs/xfs/*", \
    RUN+="/bin/sh -c 'echo 0 > /sys%p/error/metadata/default/max_retries; \
	    echo 0 > /sys%p/error/metadata/EIO/max_retries; \
	    echo 0 > /sys%p/error/metadata/ENOSPC/max_retries; \
	    echo 0 > /sys%p/error/metadata/ENODEV/max_retries'"

You also could apply an udevd rule to a specific XFS filesystem by
UUID filtering:

    ACTION=="online", SUBSYSTEM=="xfs", \
    ENV{UUID}=="8f789c27-391f-4bd7-b17d-9bcf2443dc9c", \
    RUN+="/bin/sh -c 'echo 5 > /sys%p/error/metadata/EIO/max_retries'"

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
v2:
    * add UUID property for mount uevent
    * add an udev example for UUID filtering
v1:
    * http://www.spinics.net/lists/linux-xfs/msg09484.html
---
 fs/xfs/xfs_super.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

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_UUID_LEN = UUID_STRING_LEN + 6,
+};
+
 /*
  * 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;
+
+	if (!uuid_is_null(&mp->m_super->s_uuid)) {
+		char uuid[XFS_UEVENT_UUID_LEN];
+
+		snprintf(uuid, sizeof(uuid), "UUID=%pUb", &mp->m_super->s_uuid);
+		envp[i++] = uuid;
+	}
+	envp[i] = NULL;
+	err = kobject_uevent_env(&mp->m_kobj.kobject, action, envp);
+	if (err)
+		xfs_notice(mp, "Sending XFS uevent %d got error %d",
+				action, err);
+}
+
 STATIC int
 xfs_fs_fill_super(
 	struct super_block	*sb,
@@ -1667,6 +1694,8 @@ xfs_fs_fill_super(
 		goto out_unmount;
 	}
 
+	xfs_fs_uevent(mp, KOBJ_ONLINE);
+
 	return 0;
 
  out_filestream_unmount:
-- 
2.5.0

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

[Darrick J. Wong]

On Thu, Aug 31, 2017 at 10:17:10PM +0800, Hou Tao wrote:
> It will be useful if there is a corresponding online uevent after
> a XFS filesystem has been mounted. A typical usage of the uevent
> is setting the error configuration for a specific XFS filesystem
> or all XFS filesystems by using udevd.
> 
> The following is an example of udevd rule which will shutdown
> any XFS filesystem after the filesystem gets any IO error:
> 
>     ACTION=="online", SUBSYSTEM=="xfs", DEVPATH=="/fs/xfs/*", \
>     RUN+="/bin/sh -c 'echo 0 > /sys%p/error/metadata/default/max_retries; \
> 	    echo 0 > /sys%p/error/metadata/EIO/max_retries; \
> 	    echo 0 > /sys%p/error/metadata/ENOSPC/max_retries; \
> 	    echo 0 > /sys%p/error/metadata/ENODEV/max_retries'"
> 
> You also could apply an udevd rule to a specific XFS filesystem by
> UUID filtering:
> 
>     ACTION=="online", SUBSYSTEM=="xfs", \
>     ENV{UUID}=="8f789c27-391f-4bd7-b17d-9bcf2443dc9c", \
>     RUN+="/bin/sh -c 'echo 5 > /sys%p/error/metadata/EIO/max_retries'"
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> v2:
>     * add UUID property for mount uevent
>     * add an udev example for UUID filtering
> v1:
>     * http://www.spinics.net/lists/linux-xfs/msg09484.html
> ---
>  fs/xfs/xfs_super.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> 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 ?

> +	XFS_UEVENT_UUID_LEN = UUID_STRING_LEN + 6,
> +};
> +
>  /*
>   * 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;
> +
> +	if (!uuid_is_null(&mp->m_super->s_uuid)) {
> +		char uuid[XFS_UEVENT_UUID_LEN];
> +
> +		snprintf(uuid, sizeof(uuid), "UUID=%pUb", &mp->m_super->s_uuid);

Should this be "ID_FS_UUID=%pUb" to keep the name consistent with what
blkid injects into the udev environment for block devices?

--D

> +		envp[i++] = uuid;
> +	}
> +	envp[i] = NULL;
> +	err = kobject_uevent_env(&mp->m_kobj.kobject, action, envp);
> +	if (err)
> +		xfs_notice(mp, "Sending XFS uevent %d got error %d",
> +				action, err);
> +}
> +
>  STATIC int
>  xfs_fs_fill_super(
>  	struct super_block	*sb,
> @@ -1667,6 +1694,8 @@ xfs_fs_fill_super(
>  		goto out_unmount;
>  	}
>  
> +	xfs_fs_uevent(mp, KOBJ_ONLINE);
> +
>  	return 0;
>  
>   out_filestream_unmount:
> -- 
> 2.5.0
> 
> --
> 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 v2] xfs: add online uevent for mount operation

[Dave Chinner]

On Thu, Aug 31, 2017 at 08:03:34AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 31, 2017 at 10:17:10PM +0800, Hou Tao wrote:
> > It will be useful if there is a corresponding online uevent after
> > a XFS filesystem has been mounted. A typical usage of the uevent
> > is setting the error configuration for a specific XFS filesystem
> > or all XFS filesystems by using udevd.
> > 
> > The following is an example of udevd rule which will shutdown
> > any XFS filesystem after the filesystem gets any IO error:
> > 
> >     ACTION=="online", SUBSYSTEM=="xfs", DEVPATH=="/fs/xfs/*", \
> >     RUN+="/bin/sh -c 'echo 0 > /sys%p/error/metadata/default/max_retries; \
> > 	    echo 0 > /sys%p/error/metadata/EIO/max_retries; \
> > 	    echo 0 > /sys%p/error/metadata/ENOSPC/max_retries; \
> > 	    echo 0 > /sys%p/error/metadata/ENODEV/max_retries'"
> > 
> > You also could apply an udevd rule to a specific XFS filesystem by
> > UUID filtering:
> > 
> >     ACTION=="online", SUBSYSTEM=="xfs", \
> >     ENV{UUID}=="8f789c27-391f-4bd7-b17d-9bcf2443dc9c", \
> >     RUN+="/bin/sh -c 'echo 5 > /sys%p/error/metadata/EIO/max_retries'"

Nice!

> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
> > ---
> > v2:
> >     * add UUID property for mount uevent
> >     * add an udev example for UUID filtering
> > v1:
> >     * http://www.spinics.net/lists/linux-xfs/msg09484.html
> > ---
> >  fs/xfs/xfs_super.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > 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?

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

> > +};
> > +
> >  /*
> >   * 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.

> > +
> > +	if (!uuid_is_null(&mp->m_super->s_uuid)) {

This will never be false. XFS filesystems should always have a valid
UUID.

> > +		char uuid[XFS_UEVENT_UUID_LEN];
> > +
> > +		snprintf(uuid, sizeof(uuid), "UUID=%pUb", &mp->m_super->s_uuid);

That buffer is not a uuid. i.e. this code looks to me like it is
encoding a string larger than a uuid into a uuid that is the
size of a uuid. Perhaps something like this?

		char *id = "ID_FS_UUID=";
		char buf[UUID_STRING_LEN + strlen(id) + 1] = {};

		snprintf(buf, sizeof(buf), "%s%pUb", id, &mp->m_super->s_uuid);

> Should this be "ID_FS_UUID=%pUb" to keep the name consistent with
> what blkid injects into the udev environment for block devices?

Sounds like a good idea to me...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Hou Tao]

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.

> 
>>> +		char uuid[XFS_UEVENT_UUID_LEN];
>>> +
>>> +		snprintf(uuid, sizeof(uuid), "UUID=%pUb", &mp->m_super->s_uuid);
> 
> That buffer is not a uuid. i.e. this code looks to me like it is
> encoding a string larger than a uuid into a uuid that is the
> size of a uuid. Perhaps something like this?
> 
> 		char *id = "ID_FS_UUID=";
> 		char buf[UUID_STRING_LEN + strlen(id) + 1] = {};
> 
> 		snprintf(buf, sizeof(buf), "%s%pUb", id, &mp->m_super->s_uuid);
> 
Accepted, much clearer than the original code.

>> Should this be "ID_FS_UUID=%pUb" to keep the name consistent with
>> what blkid injects into the udev environment for block devices?
> 
> Sounds like a good idea to me...
OK, I will check blkid and fix the name of the uuid environment accordingly.

Tao

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

[Dave Chinner]

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

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

[Hou Tao]

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

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

[Darrick J. Wong]

On Fri, Sep 01, 2017 at 01:11:09PM +0800, Hou Tao wrote:
> 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.

I was /about/ to reply with "Why does it matter if the UUID is null?
That's just another value, albeit a weird one.", but then I actually
tried it:

$ truncate -s 500m /tmp/a
$ mkfs.xfs -m uuid=00000000-0000-0000-0000-000000000000 -f /tmp/a
$ sudo mount /tmp/a /mnt
[161296.189297] XFS (loop0): Filesystem has nil UUID - can't mount
$ sudo mount /tmp/a /mnt -o nouuid
[161301.335715] XFS (loop0): Mounting V5 Filesystem
[161301.335881] XFS (loop0): nil uuid in log - IRIX style log
[161301.338524] XFS (loop0): Ending clean mount
[161311.233536] XFS (loop0): Unmounting Filesystem

Now I'm wondering just what that's all about, especially on a v5 fs? :)

Getting back to the original conversation, it doesn't matter if the UUID
is all zeroes; one can certainly mkfs such a filesystem.  Send out
the UUID with the uevent even if it is all zeroes.

--D

> 
> Regards,
> 
> Tao
> 
> --
> 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 v2] xfs: add online uevent for mount operation

[Dave Chinner]

On Fri, Sep 01, 2017 at 08:20:16AM -0700, Darrick J. Wong wrote:
> On Fri, Sep 01, 2017 at 01:11:09PM +0800, Hou Tao wrote:
> > > 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

IMO, that's a bug, because...

> > So I still think the null check in xfs_fs_uevent is needed.
> 
> I was /about/ to reply with "Why does it matter if the UUID is null?
> That's just another value, albeit a weird one.", but then I actually
> tried it:
> 
> $ truncate -s 500m /tmp/a
> $ mkfs.xfs -m uuid=00000000-0000-0000-0000-000000000000 -f /tmp/a
> $ sudo mount /tmp/a /mnt
> [161296.189297] XFS (loop0): Filesystem has nil UUID - can't mount
> $ sudo mount /tmp/a /mnt -o nouuid
> [161301.335715] XFS (loop0): Mounting V5 Filesystem
> [161301.335881] XFS (loop0): nil uuid in log - IRIX style log
> [161301.338524] XFS (loop0): Ending clean mount
> [161311.233536] XFS (loop0): Unmounting Filesystem
> 
> Now I'm wondering just what that's all about, especially on a v5 fs? :)

... uuids were added to the log to identify external log devices on
Linux. THey weren't needed on Irix because external log devices were
set up through the volume manager (XLV/XVM) and discovered at mount
time by the kernel code (libxfs/mkfs still has support for
"volume logs"). On linux, we have an external block device, so
something needed to be added to the log record headers so the
filesystem could determine it was about to replay the correct log
device....

IOWs finding a log with a null uuid is a big red flag that something
is not right with the filesystem setup, and we should definitely not
be mounting it if it's a v5 filesystem (uuid is mandatory in that
format). For v4, well, irix is long gone, as are the vast majority
of Irix XFS filesystems, so we should probably not mount them,
either.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Sat, Sep 02, 2017 at 07:52:36AM +1000, Dave Chinner wrote:
> On Fri, Sep 01, 2017 at 08:20:16AM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 01, 2017 at 01:11:09PM +0800, Hou Tao wrote:
> > > > 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
> 
> IMO, that's a bug, because...
> 
> > > So I still think the null check in xfs_fs_uevent is needed.
> > 
> > I was /about/ to reply with "Why does it matter if the UUID is null?
> > That's just another value, albeit a weird one.", but then I actually
> > tried it:
> > 
> > $ truncate -s 500m /tmp/a
> > $ mkfs.xfs -m uuid=00000000-0000-0000-0000-000000000000 -f /tmp/a
> > $ sudo mount /tmp/a /mnt
> > [161296.189297] XFS (loop0): Filesystem has nil UUID - can't mount
> > $ sudo mount /tmp/a /mnt -o nouuid
> > [161301.335715] XFS (loop0): Mounting V5 Filesystem
> > [161301.335881] XFS (loop0): nil uuid in log - IRIX style log
> > [161301.338524] XFS (loop0): Ending clean mount
> > [161311.233536] XFS (loop0): Unmounting Filesystem
> > 
> > Now I'm wondering just what that's all about, especially on a v5 fs? :)
> 
> ... uuids were added to the log to identify external log devices on
> Linux. THey weren't needed on Irix because external log devices were
> set up through the volume manager (XLV/XVM) and discovered at mount
> time by the kernel code (libxfs/mkfs still has support for
> "volume logs"). On linux, we have an external block device, so
> something needed to be added to the log record headers so the
> filesystem could determine it was about to replay the correct log
> device....
> 
> IOWs finding a log with a null uuid is a big red flag that something
> is not right with the filesystem setup, and we should definitely not
> be mounting it if it's a v5 filesystem (uuid is mandatory in that
> format). For v4, well, irix is long gone, as are the vast majority
> of Irix XFS filesystems, so we should probably not mount them,
> either.

<nod> Patches to reject zero-uuid filesystems welcome. ;)

That said, I think that the uevent should include ID_FS_UUID regardless
of the fsuuid contents; it's the job of the mount code to decide if the
uuid is screwy and therefore we should scuttle everything.  I also
decided to defer this one to 4.15 on the grounds that if the purpose of
adding a uevent is to get a userspace helper to read in the xfs config,
let's see at least a draft of that piece on the mailing list first.

TBH I'm also a little confused about the repost of the default config
values patchset immediately prior to this patch -- if the goal is to
delegate loading default vs. fs-specific configuration policy to a
userland daemon via uevent, then why would we need a sysfs directory of
default config knobs in addition to the existing per-fs knobs?

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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 v2] xfs: add online uevent for mount operation

[Hou Tao]

Hi Darrick,

On 2017/9/4 8:03, Darrick J. Wong wrote:
> On Sat, Sep 02, 2017 at 07:52:36AM +1000, Dave Chinner wrote:
>> On Fri, Sep 01, 2017 at 08:20:16AM -0700, Darrick J. Wong wrote:
>>> On Fri, Sep 01, 2017 at 01:11:09PM +0800, Hou Tao wrote:
>>>>> 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
>>
>> IMO, that's a bug, because...
>>
>>>> So I still think the null check in xfs_fs_uevent is needed.
>>>
>>> I was /about/ to reply with "Why does it matter if the UUID is null?
>>> That's just another value, albeit a weird one.", but then I actually
>>> tried it:
>>>
>>> $ truncate -s 500m /tmp/a
>>> $ mkfs.xfs -m uuid=00000000-0000-0000-0000-000000000000 -f /tmp/a
>>> $ sudo mount /tmp/a /mnt
>>> [161296.189297] XFS (loop0): Filesystem has nil UUID - can't mount
>>> $ sudo mount /tmp/a /mnt -o nouuid
>>> [161301.335715] XFS (loop0): Mounting V5 Filesystem
>>> [161301.335881] XFS (loop0): nil uuid in log - IRIX style log
>>> [161301.338524] XFS (loop0): Ending clean mount
>>> [161311.233536] XFS (loop0): Unmounting Filesystem
>>>
>>> Now I'm wondering just what that's all about, especially on a v5 fs? :)
>>
>> ... uuids were added to the log to identify external log devices on
>> Linux. THey weren't needed on Irix because external log devices were
>> set up through the volume manager (XLV/XVM) and discovered at mount
>> time by the kernel code (libxfs/mkfs still has support for
>> "volume logs"). On linux, we have an external block device, so
>> something needed to be added to the log record headers so the
>> filesystem could determine it was about to replay the correct log
>> device....
>>
>> IOWs finding a log with a null uuid is a big red flag that something
>> is not right with the filesystem setup, and we should definitely not
>> be mounting it if it's a v5 filesystem (uuid is mandatory in that
>> format). For v4, well, irix is long gone, as are the vast majority
>> of Irix XFS filesystems, so we should probably not mount them,
>> either.
> 
> <nod> Patches to reject zero-uuid filesystems welcome. ;)
> 
> That said, I think that the uevent should include ID_FS_UUID regardless
> of the fsuuid contents; it's the job of the mount code to decide if the
> uuid is screwy and therefore we should scuttle everything.  I also
> decided to defer this one to 4.15 on the grounds that if the purpose of
> adding a uevent is to get a userspace helper to read in the xfs config,
> let's see at least a draft of that piece on the mailing list first.
> 
> TBH I'm also a little confused about the repost of the default config
> values patchset immediately prior to this patch -- if the goal is to
> delegate loading default vs. fs-specific configuration policy to a
> userland daemon via uevent, then why would we need a sysfs directory of
> default config knobs in addition to the existing per-fs knobs?
One scenario I think the default config sysfs files are needed is embedded
environment under which udevd or a similar daemon is not available. It also
provides a shortcut for the "default" configuration options which can be set
in one time instead of one-by-one, but I'm not sure these "advantages" is worthy
of the workload of code.

I should have asked Dave for the explanation before the reworking of the patchset,
so Dave, Any explanation or ideas ?

Regards,

Tao


> --D
> 
>>
>> Cheers,
>>
>> Dave.
>> -- 
>> Dave Chinner
>> david@fromorbit.com
>> --
>> 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
> 
> .
>