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

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

On Wed, Jun 21, 2017 at 02:19:18PM -0400, Brian Foster wrote:
> On Tue, Jun 20, 2017 at 06:11:24PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Creates a /sys/fs/xfs/$dev/errortag/ directory to control the errortag
> > values directly.  This enables us to control the randomness values,
> > rather than having to accept the defaults.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_error.c |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_error.h |    1 
> >  fs/xfs/xfs_mount.h |    1 
> >  3 files changed, 157 insertions(+), 1 deletion(-)
> > 
> > 
> > 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);
> > +}
> > +
> > +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.

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?

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