| Message ID | 1504189030-18608-1-git-send-email-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 -- 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
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.
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 -- 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
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.
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 -- 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
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 -- 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
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.
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 -- 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
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 > > . > -- 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
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:
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(+)