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@xxxxxxxxxxxxx>
> > Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
