Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Jun 22, 2017 at 05:15:57PM +0200, Carlos Maiolino wrote:
> Hi:
> 
> > > > > > 
> > > > > > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > > > > > index ef16ac2..81f260f 100644
> > > > > > --- a/fs/xfs/xfs_error.c
> > > > > > +++ b/fs/xfs/xfs_error.c
> > > > > > @@ -22,6 +22,7 @@
> > > > > >  #include "xfs_trans_resv.h"
> > > > > >  #include "xfs_mount.h"
> > > > > >  #include "xfs_error.h"
> > > > > > +#include "xfs_sysfs.h"
> > > > > >  
> > > > > >  #ifdef DEBUG
> > > > > >  
> > > > > > @@ -56,6 +57,145 @@ static unsigned int xfs_errortag_random_default[] = {
> > > > > >  	XFS_RANDOM_AG_RESV_CRITICAL,
> > > > > >  };
> > > > > >  
> > > > > > +struct xfs_errortag_attr {
> > > > > > +	struct attribute	attr;
> > > > > > +	unsigned int		tag;
> > > > > > +};
> > > > > > +
> > > > > > +static inline struct xfs_errortag_attr *
> > > > > > +to_attr(struct attribute *attr)
> > > > > > +{
> > > > > > +	return container_of(attr, struct xfs_errortag_attr, attr);
> > > > > > +}
> > > > > > +
> > > > > > +static inline struct xfs_mount *
> > > > > > +to_mp(struct kobject *kobject)
> > > > > > +{
> > > > > > +	struct xfs_kobj *kobj = to_kobj(kobject);
> > > > > > +
> > > > > > +	return container_of(kobj, struct xfs_mount, m_errortag_kobj);
> > > > > > +}
> > > > > > 
> 
> Wouldn't be better to move these stuff into xfs_sysfs.c? We already
> have there such macros, or, if keeping debug stuff into xfs_error.c
> looks more sane than adding the #ifdef DEBUG to xfs_sysfs.c maybe
> moving those common macros/inlines (such as to_mp()) to a common
> header makes more sense?

But they're not the same to_mp and to_attr -- we file all the debugging
knobs under errortag/, which is a separate kobj from the xfs_mount one.
to_attr is different because we've created a new structure with a tag
number that also avoids the store/show function pointer redirection of
the regular xfs_attr structure.  I could refactor xfs_attr to store a
tag and update all the related functions, but for an initial RFC I
didn't want to spend time wading through all the macros.

I chose to keep all the errortag stuff together in xfs_error.c since all
the code is related functionality.  That's mostly a reflection of my
competing distastes for spreading tuning knob code over multiple files
vs. letting all the sysfs macro gooeyness spread everywhere.  Since the
tuning knobs are all a class unto themselves, I figured it was fine to
put them in xfs_error.c

> > > > > > +
> > > > > > +STATIC ssize_t
> > > > > > +xfs_errortag_attr_store(
> > > > > > +	struct kobject		*kobject,
> > > > > > +	struct attribute	*attr,
> > > > > > +	const char		*buf,
> > > > > > +	size_t			count)
> > > > > > +{
> > > > > > +	struct xfs_mount	*mp = to_mp(kobject);
> > > > > > +	struct xfs_errortag_attr *xfs_attr = to_attr(attr);
> > > > > > +	int			ret;
> > > > > > +	unsigned int		val;
> > > > > > +
> > > > > > +	if (strcmp(buf, "on") == 0) {
> > > > > > +		val = xfs_errortag_random_default[xfs_attr->tag];
> > > > > 
> > > > > I'm also wondering if we really care to preserve this. It doesn't seem
> > > > > like something we would add if we were adding this mechanism from
> > > > > scratch, for example, but I could be wrong. Obviously this isn't from
> > > > > scratch, but I kind of view this as morphing the old mechanism into
> > > > > something slightly new (while preserving the old interface).
> > > > 
> > > > I debated whether or not to port every aspect of the old ioctl to this
> > > > new interface.  My thought was that the current ioctl applies whatever
> > > > default the kernel has, so therefore the sysfs interface needs a kludge
> > > > to preserve that feature because the current crop of test cases accept
> > > > the kernel defaults.  We /could/ change the xfstests helper to supply
> > > > the default value to sysfs if the test doesn't otherwise provide a
> > > > value, but on an old kernel there's no way to figure out if the value
> > > > set was the value you wanted, so if the value you supply is the same as
> > > > the encoded default then try the ioctl and if it succeeds then maybe the
> > > > injection is set up the way the test wants?  Ick.
> > > > 
> > > 
> > > I'm a little confused... is there any reason we can't keep the ioctl()
> > > interface around (and in use by the existing xfstests helpers) for the
> > > purpose of preserving this traditional "hardcoded default" behavior?
> > > Anything that uses/expects kernel provided settings can just continue to
> > > use the existing interface. Anything new or that wants finer grained
> > > control can use sysfs.
> > 
> > I think I confused myself and everyone else about this point.  I wasn't
> > planning to get rid of the old ioctl, but then went off into the weeds
> > over "what if we ever did remove it".
> > 
> > Sooo... this is my vision:
> > 
> > The ioctl will retain the behavior that you can only set the value to
> > the default; it will not let you set the value if it's already been
> > turned on; and the only way to turn off any of the error tags is to
> > clear all of them.
> > 
> > The sysfs interface will let you set the values to anything you want at
> > any time, including 0 (off) or "default".
> > 
> 
> I don't want to add more fire to it, but, do we gain any benefit in
> keeping both interfaces? It just sounds confusing to me. Bear in mind
> though, I've never played with the ioctl() interface, that's why I'm
> asking if there is any benefit in keeping them both :)

It depends on how we handle userspace.  It's tempting to deprecate the
ioctl in favor of setting the sysfs knobs and switch xfstests over to
the sysfs knobs via a bash function helper that figures out which
interface is available.

That said, if we /do/ decide to drop the ioctl, we ought to have a
deprecation schedule to give people time to absorb new xfstests.  Being
a debug feature that schedule can be much shorter than usual, but I
don't want to have a flag day just for this. :)

(FWIW I hate the ioctl interface and want it to die.)

--D

> 
> cheers
> 
> > > > Then again it's a debug interface that historically didn't check the
> > > > error tag being input either, so.... :(
> > > > 
> > > > Also, maybe "default" would have been a better choice for the value?
> > > > 
> > > 
> > > I do like "default" better than "on" if we want to keep the ability to
> > > set the associated value via sysfs.
> > 
> > Ok.
> > 
> > --D
> > 
> > > 
> > > Brian
> > > 
> > > > --D
> > > > 
> > > > > I'll probably need to stare a bit more at the sysfs bits, but otherwise
> > > > > the rest all seems fine to me at first glance.
> > > > > 
> > > > > Brian
> > > > > 
> > > > > > +	} else {
> > > > > > +		ret = kstrtouint(buf, 0, &val);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = xfs_errortag_set(mp, xfs_attr->tag, val);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +	return count;
> > > > > > +}
> > > > > > +
> > > > > > +STATIC ssize_t
> > > > > > +xfs_errortag_attr_show(
> > > > > > +	struct kobject		*kobject,
> > > > > > +	struct attribute	*attr,
> > > > > > +	char			*buf)
> > > > > > +{
> > > > > > +	struct xfs_mount	*mp = to_mp(kobject);
> > > > > > +	struct xfs_errortag_attr *xfs_attr = to_attr(attr);
> > > > > > +
> > > > > > +	return snprintf(buf, PAGE_SIZE, "%u\n",
> > > > > > +			xfs_errortag_get(mp, xfs_attr->tag));
> > > > > > +}
> > > > > > +
> > > > > > +static const struct sysfs_ops xfs_errortag_sysfs_ops = {
> > > > > > +	.show = xfs_errortag_attr_show,
> > > > > > +	.store = xfs_errortag_attr_store,
> > > > > > +};
> > > > > > +
> > > > > > +#define XFS_ERRORTAG_ATTR_RW(_name, _tag) \
> > > > > > +static struct xfs_errortag_attr xfs_errortag_attr_##_name = {		\
> > > > > > +	.attr = {.name = __stringify(_name),				\
> > > > > > +		 .mode = VERIFY_OCTAL_PERMISSIONS(S_IWUSR | S_IRUGO) },	\
> > > > > > +	.tag	= (_tag),						\
> > > > > > +}
> > > > > > +
> > > > > > +#define XFS_ERRORTAG_ATTR_LIST(_name) &xfs_errortag_attr_##_name.attr
> > > > > > +
> > > > > > +XFS_ERRORTAG_ATTR_RW(noerror,		XFS_ERRTAG_NOERROR);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iflush1,		XFS_ERRTAG_IFLUSH_1);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iflush2,		XFS_ERRTAG_IFLUSH_2);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iflush3,		XFS_ERRTAG_IFLUSH_3);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iflush4,		XFS_ERRTAG_IFLUSH_4);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iflush5,		XFS_ERRTAG_IFLUSH_5);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iflush6,		XFS_ERRTAG_IFLUSH_6);
> > > > > > +XFS_ERRORTAG_ATTR_RW(dareadbuf,		XFS_ERRTAG_DA_READ_BUF);
> > > > > > +XFS_ERRORTAG_ATTR_RW(btree_chk_lblk,	XFS_ERRTAG_BTREE_CHECK_LBLOCK);
> > > > > > +XFS_ERRORTAG_ATTR_RW(btree_chk_sblk,	XFS_ERRTAG_BTREE_CHECK_SBLOCK);
> > > > > > +XFS_ERRORTAG_ATTR_RW(readagf,		XFS_ERRTAG_ALLOC_READ_AGF);
> > > > > > +XFS_ERRORTAG_ATTR_RW(readagi,		XFS_ERRTAG_IALLOC_READ_AGI);
> > > > > > +XFS_ERRORTAG_ATTR_RW(itobp,		XFS_ERRTAG_ITOBP_INOTOBP);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iunlink,		XFS_ERRTAG_IUNLINK);
> > > > > > +XFS_ERRORTAG_ATTR_RW(iunlinkrm,		XFS_ERRTAG_IUNLINK_REMOVE);
> > > > > > +XFS_ERRORTAG_ATTR_RW(dirinovalid,	XFS_ERRTAG_DIR_INO_VALIDATE);
> > > > > > +XFS_ERRORTAG_ATTR_RW(bulkstat,		XFS_ERRTAG_BULKSTAT_READ_CHUNK);
> > > > > > +XFS_ERRORTAG_ATTR_RW(logiodone,		XFS_ERRTAG_IODONE_IOERR);
> > > > > > +XFS_ERRORTAG_ATTR_RW(stratread,		XFS_ERRTAG_STRATREAD_IOERR);
> > > > > > +XFS_ERRORTAG_ATTR_RW(stratcmpl,		XFS_ERRTAG_STRATCMPL_IOERR);
> > > > > > +XFS_ERRORTAG_ATTR_RW(diowrite,		XFS_ERRTAG_DIOWRITE_IOERR);
> > > > > > +XFS_ERRORTAG_ATTR_RW(bmapifmt,		XFS_ERRTAG_BMAPIFORMAT);
> > > > > > +XFS_ERRORTAG_ATTR_RW(free_extent,	XFS_ERRTAG_FREE_EXTENT);
> > > > > > +XFS_ERRORTAG_ATTR_RW(rmap_finish_one,	XFS_ERRTAG_RMAP_FINISH_ONE);
> > > > > > +XFS_ERRORTAG_ATTR_RW(refcount_continue_update,	XFS_ERRTAG_REFCOUNT_CONTINUE_UPDATE);
> > > > > > +XFS_ERRORTAG_ATTR_RW(refcount_finish_one,	XFS_ERRTAG_REFCOUNT_FINISH_ONE);
> > > > > > +XFS_ERRORTAG_ATTR_RW(bmap_finish_one,	XFS_ERRTAG_BMAP_FINISH_ONE);
> > > > > > +XFS_ERRORTAG_ATTR_RW(ag_resv_critical,	XFS_ERRTAG_AG_RESV_CRITICAL);
> > > > > > +
> > > > > > +static struct attribute *xfs_errortag_attrs[] = {
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(noerror),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iflush1),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iflush2),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iflush3),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iflush4),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iflush5),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iflush6),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(dareadbuf),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(btree_chk_lblk),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(btree_chk_sblk),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(readagf),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(readagi),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(itobp),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iunlink),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(iunlinkrm),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(dirinovalid),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(bulkstat),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(logiodone),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(stratread),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(stratcmpl),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(diowrite),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(bmapifmt),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(free_extent),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(rmap_finish_one),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(refcount_continue_update),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(refcount_finish_one),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(bmap_finish_one),
> > > > > > +	XFS_ERRORTAG_ATTR_LIST(ag_resv_critical),
> > > > > > +	NULL,
> > > > > > +};
> > > > > > +
> > > > > > +struct kobj_type xfs_errortag_ktype = {
> > > > > > +	.release = xfs_sysfs_release,
> > > > > > +	.sysfs_ops = &xfs_errortag_sysfs_ops,
> > > > > > +	.default_attrs = xfs_errortag_attrs,
> > > > > > +};
> > > > > > +
> > > > > >  int
> > > > > >  xfs_errortag_init(
> > > > > >  	struct xfs_mount	*mp)
> > > > > > @@ -64,13 +204,16 @@ xfs_errortag_init(
> > > > > >  			KM_SLEEP | KM_MAYFAIL);
> > > > > >  	if (!mp->m_errortag)
> > > > > >  		return -ENOMEM;
> > > > > > -	return 0;
> > > > > > +
> > > > > > +	return xfs_sysfs_init(&mp->m_errortag_kobj, &xfs_errortag_ktype,
> > > > > > +			       &mp->m_kobj, "errortag");
> > > > > >  }
> > > > > >  
> > > > > >  void
> > > > > >  xfs_errortag_del(
> > > > > >  	struct xfs_mount	*mp)
> > > > > >  {
> > > > > > +	xfs_sysfs_del(&mp->m_errortag_kobj);
> > > > > >  	kmem_free(mp->m_errortag);
> > > > > >  }
> > > > > >  
> > > > > > @@ -96,6 +239,17 @@ xfs_errortag_test(
> > > > > >  }
> > > > > >  
> > > > > >  int
> > > > > > +xfs_errortag_get(
> > > > > > +	struct xfs_mount	*mp,
> > > > > > +	unsigned int		error_tag)
> > > > > > +{
> > > > > > +	if (error_tag >= XFS_ERRTAG_MAX)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return mp->m_errortag[error_tag];
> > > > > > +}
> > > > > > +
> > > > > > +int
> > > > > >  xfs_errortag_set(
> > > > > >  	struct xfs_mount	*mp,
> > > > > >  	unsigned int		error_tag,
> > > > > > diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> > > > > > index 341e8a0..a3c0c1d 100644
> > > > > > --- a/fs/xfs/xfs_error.h
> > > > > > +++ b/fs/xfs/xfs_error.h
> > > > > > @@ -138,6 +138,7 @@ extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
> > > > > >  #define XFS_TEST_ERROR(expr, mp, tag, rf)		\
> > > > > >  	((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
> > > > > >  
> > > > > > +extern int xfs_errortag_get(struct xfs_mount *mp, unsigned int error_tag);
> > > > > >  extern int xfs_errortag_set(struct xfs_mount *mp, unsigned int error_tag,
> > > > > >  		unsigned int tag_value);
> > > > > >  extern int xfs_errortag_add(struct xfs_mount *mp, unsigned int error_tag);
> > > > > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > > > > index e002ac5..931e9fc 100644
> > > > > > --- a/fs/xfs/xfs_mount.h
> > > > > > +++ b/fs/xfs/xfs_mount.h
> > > > > > @@ -204,6 +204,7 @@ typedef struct xfs_mount {
> > > > > >  	 * error triggers.  1 = always, 2 = half the time, etc.
> > > > > >  	 */
> > > > > >  	unsigned int		*m_errortag;
> > > > > > +	struct xfs_kobj		m_errortag_kobj;
> > > > > >  
> > > > > >  	/*
> > > > > >  	 * DEBUG mode instrumentation to test and/or trigger delayed allocation
> > > > > > 
> > > > > > --
> > > > > > 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
> > > --
> > > 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
> 
> -- 
> Carlos
> --
> 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