* [RFC PATCH 0/4] xfs: more configurable error injection
@ 2017-06-21 1:11 Darrick J. Wong
2017-06-21 1:11 ` [PATCH 1/4] xfs: make errortag a per-mountpoint structure Darrick J. Wong
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 1:11 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
Hi all,
This is a small patch set that exposes all the error injection tags
under /sys/fs/xfs/$dev/errortag/. Each tag gets its own sysfs
attribute; you can enable a tag via:
# echo $num > /sys/fs/xfs/$dev/errortag/$tagname
The tag names are the same ones as xfs_io's inject command, and the
number units are the denominator of the occurrence frequency, which is
to say that they're the same units as XFS_RANDOM_*. 1 = all the time,
2 = half the time, 100 = 1% of the time, etc.
The purpose of this patch series is twofold -- first, the old ioctl
interface doesn't allow us to control how often errors actually get
injected. The extra flexibility enables us to write regression tests
where we set up the next operation to fail, as well as tests that set a
low frequency of trouble and try to run longer tests. Second, with
error injection knobs appearing in sysfs we might as well move them all
to the same place. Third, now we can individually /disable/ error
injectors, whereas the ioctl interface can only turn all of them off at
once.
Note that the old ioctls are not going away; their behavior is
unchanged.
Comments and questions are, as always, welcome.
--D
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] xfs: make errortag a per-mountpoint structure
2017-06-21 1:11 [RFC PATCH 0/4] xfs: more configurable error injection Darrick J. Wong
@ 2017-06-21 1:11 ` Darrick J. Wong
2017-06-21 18:18 ` Brian Foster
2017-06-21 1:11 ` [PATCH 2/4] xfs: expose errortag knobs via sysfs Darrick J. Wong
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 1:11 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Remove the xfs_etest structure in favor of a per-mountpoint structure.
This will give us the flexibility to set as many error injection points
as we want, and later enable us to set up sysfs knobs to set the trigger
frequency as we wish. This comes at a cost of higher memory use, but
unti we hit 1024 injection points (we're at 29) or a lot of mounts this
shouldn't be a huge issue.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_error.c | 161 +++++++++++++++++++++++++++++++---------------------
fs/xfs/xfs_error.h | 23 ++++---
fs/xfs/xfs_ioctl.c | 4 +
fs/xfs/xfs_mount.c | 10 +++
fs/xfs/xfs_mount.h | 7 ++
5 files changed, 125 insertions(+), 80 deletions(-)
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index ed7ee4e..ef16ac2 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -25,97 +25,126 @@
#ifdef DEBUG
-int xfs_etest[XFS_NUM_INJECT_ERROR];
-int64_t xfs_etest_fsid[XFS_NUM_INJECT_ERROR];
-char * xfs_etest_fsname[XFS_NUM_INJECT_ERROR];
-int xfs_error_test_active;
+static unsigned int xfs_errortag_random_default[] = {
+ XFS_RANDOM_DEFAULT,
+ XFS_RANDOM_IFLUSH_1,
+ XFS_RANDOM_IFLUSH_2,
+ XFS_RANDOM_IFLUSH_3,
+ XFS_RANDOM_IFLUSH_4,
+ XFS_RANDOM_IFLUSH_5,
+ XFS_RANDOM_IFLUSH_6,
+ XFS_RANDOM_DA_READ_BUF,
+ XFS_RANDOM_BTREE_CHECK_LBLOCK,
+ XFS_RANDOM_BTREE_CHECK_SBLOCK,
+ XFS_RANDOM_ALLOC_READ_AGF,
+ XFS_RANDOM_IALLOC_READ_AGI,
+ XFS_RANDOM_ITOBP_INOTOBP,
+ XFS_RANDOM_IUNLINK,
+ XFS_RANDOM_IUNLINK_REMOVE,
+ XFS_RANDOM_DIR_INO_VALIDATE,
+ XFS_RANDOM_BULKSTAT_READ_CHUNK,
+ XFS_RANDOM_IODONE_IOERR,
+ XFS_RANDOM_STRATREAD_IOERR,
+ XFS_RANDOM_STRATCMPL_IOERR,
+ XFS_RANDOM_DIOWRITE_IOERR,
+ XFS_RANDOM_BMAPIFORMAT,
+ XFS_RANDOM_FREE_EXTENT,
+ XFS_RANDOM_RMAP_FINISH_ONE,
+ XFS_RANDOM_REFCOUNT_CONTINUE_UPDATE,
+ XFS_RANDOM_REFCOUNT_FINISH_ONE,
+ XFS_RANDOM_BMAP_FINISH_ONE,
+ XFS_RANDOM_AG_RESV_CRITICAL,
+};
int
-xfs_error_test(int error_tag, int *fsidp, char *expression,
- int line, char *file, unsigned long randfactor)
+xfs_errortag_init(
+ struct xfs_mount *mp)
{
- int i;
- int64_t fsid;
+ mp->m_errortag = kmem_zalloc(sizeof(unsigned int) * XFS_ERRTAG_MAX,
+ KM_SLEEP | KM_MAYFAIL);
+ if (!mp->m_errortag)
+ return -ENOMEM;
+ return 0;
+}
- if (prandom_u32() % randfactor)
- return 0;
+void
+xfs_errortag_del(
+ struct xfs_mount *mp)
+{
+ kmem_free(mp->m_errortag);
+}
- memcpy(&fsid, fsidp, sizeof(xfs_fsid_t));
+bool
+xfs_errortag_test(
+ struct xfs_mount *mp,
+ const char *expression,
+ const char *file,
+ int line,
+ unsigned int error_tag)
+{
+ unsigned int randfactor;
- for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
- if (xfs_etest[i] == error_tag && xfs_etest_fsid[i] == fsid) {
- xfs_warn(NULL,
- "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
- expression, file, line, xfs_etest_fsname[i]);
- return 1;
- }
- }
+ ASSERT(error_tag < XFS_ERRTAG_MAX);
+ randfactor = mp->m_errortag[error_tag];
+ if (!randfactor || prandom_u32() % randfactor)
+ return false;
- return 0;
+ xfs_warn(mp,
+"Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
+ expression, file, line, mp->m_fsname);
+ return true;
}
int
-xfs_errortag_add(unsigned int error_tag, xfs_mount_t *mp)
+xfs_errortag_set(
+ struct xfs_mount *mp,
+ unsigned int error_tag,
+ unsigned int tag_value)
{
- int i;
- int len;
- int64_t fsid;
-
if (error_tag >= XFS_ERRTAG_MAX)
return -EINVAL;
- memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
+ mp->m_errortag[error_tag] = tag_value;
+ xfs_warn(mp,
+"Turned %s XFS error tag #%d",
+ tag_value ? "on" : "off", error_tag);
+ return 0;
+}
- for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
- if (xfs_etest_fsid[i] == fsid && xfs_etest[i] == error_tag) {
- xfs_warn(mp, "error tag #%d on", error_tag);
- return 0;
- }
- }
+int
+xfs_errortag_add(
+ struct xfs_mount *mp,
+ unsigned int error_tag)
+{
+ if (error_tag >= XFS_ERRTAG_MAX)
+ return -EINVAL;
- for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
- if (xfs_etest[i] == 0) {
- xfs_warn(mp, "Turned on XFS error tag #%d",
- error_tag);
- xfs_etest[i] = error_tag;
- xfs_etest_fsid[i] = fsid;
- len = strlen(mp->m_fsname);
- xfs_etest_fsname[i] = kmem_alloc(len + 1, KM_SLEEP);
- strcpy(xfs_etest_fsname[i], mp->m_fsname);
- xfs_error_test_active++;
- return 0;
- }
+ if (mp->m_errortag[error_tag]) {
+ xfs_warn(mp, "error tag #%d on", error_tag);
+ return 0;
}
- xfs_warn(mp, "error tag overflow, too many turned on");
-
- return 1;
+ return xfs_errortag_set(mp, error_tag,
+ xfs_errortag_random_default[error_tag]);
}
int
-xfs_errortag_clearall(xfs_mount_t *mp, int loud)
+xfs_errortag_clearall(
+ struct xfs_mount *mp,
+ bool loud)
{
- int64_t fsid;
- int cleared = 0;
- int i;
-
- memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
-
-
- for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
- if ((fsid == 0LL || xfs_etest_fsid[i] == fsid) &&
- xfs_etest[i] != 0) {
- cleared = 1;
- xfs_warn(mp, "Clearing XFS error tag #%d",
- xfs_etest[i]);
- xfs_etest[i] = 0;
- xfs_etest_fsid[i] = 0LL;
- kmem_free(xfs_etest_fsname[i]);
- xfs_etest_fsname[i] = NULL;
- xfs_error_test_active--;
+ bool cleared = false;
+ unsigned int i;
+
+ for (i = 0; i < XFS_ERRTAG_MAX; i++) {
+ if (mp->m_errortag[i]) {
+ cleared = true;
+ break;
}
}
+ memset(mp->m_errortag, 0, sizeof(unsigned int) * XFS_ERRTAG_MAX);
+
if (loud || cleared)
xfs_warn(mp, "Cleared all XFS error tags for filesystem");
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 05f8666..341e8a0 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -131,20 +131,23 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
#define XFS_RANDOM_AG_RESV_CRITICAL 4
#ifdef DEBUG
-extern int xfs_error_test_active;
-extern int xfs_error_test(int, int *, char *, int, char *, unsigned long);
-
-#define XFS_NUM_INJECT_ERROR 10
+extern int xfs_errortag_init(struct xfs_mount *mp);
+extern void xfs_errortag_del(struct xfs_mount *mp);
+extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
+ const char *file, int line, unsigned int error_tag);
#define XFS_TEST_ERROR(expr, mp, tag, rf) \
- ((expr) || (xfs_error_test_active && \
- xfs_error_test((tag), (mp)->m_fixedfsid, "expr", __LINE__, __FILE__, \
- (rf))))
+ ((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
-extern int xfs_errortag_add(unsigned int error_tag, struct xfs_mount *mp);
-extern int xfs_errortag_clearall(struct xfs_mount *mp, int loud);
+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);
+extern int xfs_errortag_clearall(struct xfs_mount *mp, bool loud);
#else
+#define xfs_errortag_init(mp) (0)
+#define xfs_errortag_del(mp)
#define XFS_TEST_ERROR(expr, mp, tag, rf) (expr)
-#define xfs_errortag_add(tag, mp) (ENOSYS)
+#define xfs_errortag_set(mp, tag, val) (ENOSYS)
+#define xfs_errortag_add(mp, tag) (ENOSYS)
#define xfs_errortag_clearall(mp, loud) (ENOSYS)
#endif /* DEBUG */
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 8ffe4ea..ab0894a 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -2037,14 +2037,14 @@ xfs_file_ioctl(
if (copy_from_user(&in, arg, sizeof(in)))
return -EFAULT;
- return xfs_errortag_add(in.errtag, mp);
+ return xfs_errortag_add(mp, in.errtag);
}
case XFS_IOC_ERROR_CLEARALL:
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
- return xfs_errortag_clearall(mp, 1);
+ return xfs_errortag_clearall(mp, true);
case XFS_IOC_FREE_EOFBLOCKS: {
struct xfs_fs_eofblocks eofb;
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index cc6789d..5cdcf43 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -720,10 +720,13 @@ xfs_mountfs(
if (error)
goto out_del_stats;
+ error = xfs_errortag_init(mp);
+ if (error)
+ goto out_remove_error_sysfs;
error = xfs_uuid_mount(mp);
if (error)
- goto out_remove_error_sysfs;
+ goto out_remove_errortag;
/*
* Set the minimum read and write sizes
@@ -1042,6 +1045,8 @@ xfs_mountfs(
xfs_da_unmount(mp);
out_remove_uuid:
xfs_uuid_unmount(mp);
+ out_remove_errortag:
+ xfs_errortag_del(mp);
out_remove_error_sysfs:
xfs_error_sysfs_del(mp);
out_del_stats:
@@ -1145,10 +1150,11 @@ xfs_unmountfs(
xfs_uuid_unmount(mp);
#if defined(DEBUG)
- xfs_errortag_clearall(mp, 0);
+ xfs_errortag_clearall(mp, false);
#endif
xfs_free_perag(mp);
+ xfs_errortag_del(mp);
xfs_error_sysfs_del(mp);
xfs_sysfs_del(&mp->m_stats.xs_kobj);
xfs_sysfs_del(&mp->m_kobj);
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 305d953..e002ac5 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -199,6 +199,13 @@ typedef struct xfs_mount {
bool m_fail_unmount;
#ifdef DEBUG
/*
+ * Frequency with which errors are injected. Replaces xfs_etest; the
+ * value stored in here is the inverse of the frequency with which the
+ * error triggers. 1 = always, 2 = half the time, etc.
+ */
+ unsigned int *m_errortag;
+
+ /*
* DEBUG mode instrumentation to test and/or trigger delayed allocation
* block killing in the event of failed writes. When enabled, all
* buffered writes are silenty dropped and handled as if they failed.
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-21 1:11 [RFC PATCH 0/4] xfs: more configurable error injection Darrick J. Wong
2017-06-21 1:11 ` [PATCH 1/4] xfs: make errortag a per-mountpoint structure Darrick J. Wong
@ 2017-06-21 1:11 ` Darrick J. Wong
2017-06-21 18:19 ` Brian Foster
2017-06-21 1:11 ` [PATCH 3/4] xfs: remove unneeded parameter from XFS_TEST_ERROR Darrick J. Wong
2017-06-21 1:11 ` [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism Darrick J. Wong
3 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 1:11 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
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];
+ } 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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] xfs: remove unneeded parameter from XFS_TEST_ERROR
2017-06-21 1:11 [RFC PATCH 0/4] xfs: more configurable error injection Darrick J. Wong
2017-06-21 1:11 ` [PATCH 1/4] xfs: make errortag a per-mountpoint structure Darrick J. Wong
2017-06-21 1:11 ` [PATCH 2/4] xfs: expose errortag knobs via sysfs Darrick J. Wong
@ 2017-06-21 1:11 ` Darrick J. Wong
2017-06-21 18:19 ` Brian Foster
2017-06-21 1:11 ` [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism Darrick J. Wong
3 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 1:11 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Since we moved the injected error frequency controls to the mountpoint,
we can get rid of the last argument to XFS_TEST_ERROR.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_ag_resv.c | 3 +--
fs/xfs/libxfs/xfs_alloc.c | 6 ++----
fs/xfs/libxfs/xfs_bmap.c | 13 ++++++-------
fs/xfs/libxfs/xfs_btree.c | 6 ++----
fs/xfs/libxfs/xfs_dir2.c | 3 +--
fs/xfs/libxfs/xfs_ialloc.c | 3 +--
fs/xfs/libxfs/xfs_inode_buf.c | 3 +--
fs/xfs/libxfs/xfs_refcount.c | 6 ++----
fs/xfs/libxfs/xfs_rmap.c | 3 +--
fs/xfs/xfs_error.h | 4 ++--
fs/xfs/xfs_inode.c | 11 +++++------
fs/xfs/xfs_iomap.c | 2 +-
fs/xfs/xfs_log.c | 3 +--
13 files changed, 26 insertions(+), 40 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index 33db69b..b008ff3 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -111,8 +111,7 @@ xfs_ag_resv_critical(
/* Critically low if less than 10% or max btree height remains. */
return XFS_TEST_ERROR(avail < orig / 10 || avail < XFS_BTREE_MAXLEVELS,
- pag->pag_mount, XFS_ERRTAG_AG_RESV_CRITICAL,
- XFS_RANDOM_AG_RESV_CRITICAL);
+ pag->pag_mount, XFS_ERRTAG_AG_RESV_CRITICAL);
}
/*
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index fefa8da..744dcae 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2454,8 +2454,7 @@ xfs_agf_read_verify(
!xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF))
xfs_buf_ioerror(bp, -EFSBADCRC);
else if (XFS_TEST_ERROR(!xfs_agf_verify(mp, bp), mp,
- XFS_ERRTAG_ALLOC_READ_AGF,
- XFS_RANDOM_ALLOC_READ_AGF))
+ XFS_ERRTAG_ALLOC_READ_AGF))
xfs_buf_ioerror(bp, -EFSCORRUPTED);
if (bp->b_error)
@@ -2842,8 +2841,7 @@ xfs_free_extent(
ASSERT(type != XFS_AG_RESV_AGFL);
if (XFS_TEST_ERROR(false, mp,
- XFS_ERRTAG_FREE_EXTENT,
- XFS_RANDOM_FREE_EXTENT))
+ XFS_ERRTAG_FREE_EXTENT))
return -EIO;
error = xfs_free_extent_fix_freelist(tp, agno, &agbp);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 19480ed..0a98807 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3992,7 +3992,7 @@ xfs_bmapi_read(
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+ mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT("xfs_bmapi_read", XFS_ERRLEVEL_LOW, mp);
return -EFSCORRUPTED;
}
@@ -4473,7 +4473,7 @@ xfs_bmapi_write(
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+ mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT("xfs_bmapi_write", XFS_ERRLEVEL_LOW, mp);
return -EFSCORRUPTED;
}
@@ -4694,7 +4694,7 @@ xfs_bmapi_remap(
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+ mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
return -EFSCORRUPTED;
}
@@ -6098,7 +6098,7 @@ xfs_bmap_shift_extents(
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+ mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT("xfs_bmap_shift_extents",
XFS_ERRLEVEL_LOW, mp);
return -EFSCORRUPTED;
@@ -6250,7 +6250,7 @@ xfs_bmap_split_extent_at(
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+ mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT("xfs_bmap_split_extent_at",
XFS_ERRLEVEL_LOW, mp);
return -EFSCORRUPTED;
@@ -6508,8 +6508,7 @@ xfs_bmap_finish_one(
return -EFSCORRUPTED;
if (XFS_TEST_ERROR(false, tp->t_mountp,
- XFS_ERRTAG_BMAP_FINISH_ONE,
- XFS_RANDOM_BMAP_FINISH_ONE))
+ XFS_ERRTAG_BMAP_FINISH_ONE))
return -EIO;
switch (type) {
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 2f8075a..4da85ff 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -101,8 +101,7 @@ xfs_btree_check_lblock(
be64_to_cpu(block->bb_u.l.bb_rightsib)));
if (unlikely(XFS_TEST_ERROR(!lblock_ok, mp,
- XFS_ERRTAG_BTREE_CHECK_LBLOCK,
- XFS_RANDOM_BTREE_CHECK_LBLOCK))) {
+ XFS_ERRTAG_BTREE_CHECK_LBLOCK))) {
if (bp)
trace_xfs_btree_corrupt(bp, _RET_IP_);
XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
@@ -153,8 +152,7 @@ xfs_btree_check_sblock(
block->bb_u.s.bb_rightsib;
if (unlikely(XFS_TEST_ERROR(!sblock_ok, mp,
- XFS_ERRTAG_BTREE_CHECK_SBLOCK,
- XFS_RANDOM_BTREE_CHECK_SBLOCK))) {
+ XFS_ERRTAG_BTREE_CHECK_SBLOCK))) {
if (bp)
trace_xfs_btree_corrupt(bp, _RET_IP_);
XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 2f389d36..ccf9783 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -218,8 +218,7 @@ xfs_dir_ino_validate(
agblkno != 0 &&
ioff < (1 << mp->m_sb.sb_inopblog) &&
XFS_AGINO_TO_INO(mp, agno, agino) == ino;
- if (unlikely(XFS_TEST_ERROR(!ino_ok, mp, XFS_ERRTAG_DIR_INO_VALIDATE,
- XFS_RANDOM_DIR_INO_VALIDATE))) {
+ if (unlikely(XFS_TEST_ERROR(!ino_ok, mp, XFS_ERRTAG_DIR_INO_VALIDATE))) {
xfs_warn(mp, "Invalid inode number 0x%Lx",
(unsigned long long) ino);
XFS_ERROR_REPORT("xfs_dir_ino_validate", XFS_ERRLEVEL_LOW, mp);
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index c514fe9..ffd5a15 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2552,8 +2552,7 @@ xfs_agi_read_verify(
!xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF))
xfs_buf_ioerror(bp, -EFSBADCRC);
else if (XFS_TEST_ERROR(!xfs_agi_verify(bp), mp,
- XFS_ERRTAG_IALLOC_READ_AGI,
- XFS_RANDOM_IALLOC_READ_AGI))
+ XFS_ERRTAG_IALLOC_READ_AGI))
xfs_buf_ioerror(bp, -EFSCORRUPTED);
if (bp->b_error)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 0c970cf..378f8fb 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -105,8 +105,7 @@ xfs_inode_buf_verify(
di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
xfs_dinode_good_version(mp, dip->di_version);
if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
- XFS_ERRTAG_ITOBP_INOTOBP,
- XFS_RANDOM_ITOBP_INOTOBP))) {
+ XFS_ERRTAG_ITOBP_INOTOBP))) {
if (readahead) {
bp->b_flags &= ~XBF_DONE;
xfs_buf_ioerror(bp, -EIO);
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index e170161..900ea23 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -805,8 +805,7 @@ xfs_refcount_still_have_space(
*/
if (cur->bc_private.a.priv.refc.nr_ops > 2 &&
XFS_TEST_ERROR(false, cur->bc_mp,
- XFS_ERRTAG_REFCOUNT_CONTINUE_UPDATE,
- XFS_RANDOM_REFCOUNT_CONTINUE_UPDATE))
+ XFS_ERRTAG_REFCOUNT_CONTINUE_UPDATE))
return false;
if (cur->bc_private.a.priv.refc.nr_ops == 0)
@@ -1068,8 +1067,7 @@ xfs_refcount_finish_one(
blockcount);
if (XFS_TEST_ERROR(false, mp,
- XFS_ERRTAG_REFCOUNT_FINISH_ONE,
- XFS_RANDOM_REFCOUNT_FINISH_ONE))
+ XFS_ERRTAG_REFCOUNT_FINISH_ONE))
return -EIO;
/*
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index eda275b..55c88a7 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -2087,8 +2087,7 @@ xfs_rmap_finish_one(
startoff, blockcount, state);
if (XFS_TEST_ERROR(false, mp,
- XFS_ERRTAG_RMAP_FINISH_ONE,
- XFS_RANDOM_RMAP_FINISH_ONE))
+ XFS_ERRTAG_RMAP_FINISH_ONE))
return -EIO;
/*
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index a3c0c1d..083a6fc 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -135,7 +135,7 @@ extern int xfs_errortag_init(struct xfs_mount *mp);
extern void xfs_errortag_del(struct xfs_mount *mp);
extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
const char *file, int line, unsigned int error_tag);
-#define XFS_TEST_ERROR(expr, mp, tag, rf) \
+#define XFS_TEST_ERROR(expr, mp, tag) \
((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
extern int xfs_errortag_get(struct xfs_mount *mp, unsigned int error_tag);
@@ -146,7 +146,7 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp, bool loud);
#else
#define xfs_errortag_init(mp) (0)
#define xfs_errortag_del(mp)
-#define XFS_TEST_ERROR(expr, mp, tag, rf) (expr)
+#define XFS_TEST_ERROR(expr, mp, tag) (expr)
#define xfs_errortag_set(mp, tag, val) (ENOSYS)
#define xfs_errortag_add(mp, tag) (ENOSYS)
#define xfs_errortag_clearall(mp, loud) (ENOSYS)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ffbfe7d..67bd563 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3489,7 +3489,7 @@ xfs_iflush_int(
dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
if (XFS_TEST_ERROR(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC),
- mp, XFS_ERRTAG_IFLUSH_1, XFS_RANDOM_IFLUSH_1)) {
+ mp, XFS_ERRTAG_IFLUSH_1)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: Bad inode %Lu magic number 0x%x, ptr 0x%p",
__func__, ip->i_ino, be16_to_cpu(dip->di_magic), dip);
@@ -3499,7 +3499,7 @@ xfs_iflush_int(
if (XFS_TEST_ERROR(
(ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
(ip->i_d.di_format != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_IFLUSH_3, XFS_RANDOM_IFLUSH_3)) {
+ mp, XFS_ERRTAG_IFLUSH_3)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: Bad regular inode %Lu, ptr 0x%p",
__func__, ip->i_ino, ip);
@@ -3510,7 +3510,7 @@ xfs_iflush_int(
(ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
(ip->i_d.di_format != XFS_DINODE_FMT_BTREE) &&
(ip->i_d.di_format != XFS_DINODE_FMT_LOCAL),
- mp, XFS_ERRTAG_IFLUSH_4, XFS_RANDOM_IFLUSH_4)) {
+ mp, XFS_ERRTAG_IFLUSH_4)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: Bad directory inode %Lu, ptr 0x%p",
__func__, ip->i_ino, ip);
@@ -3518,8 +3518,7 @@ xfs_iflush_int(
}
}
if (XFS_TEST_ERROR(ip->i_d.di_nextents + ip->i_d.di_anextents >
- ip->i_d.di_nblocks, mp, XFS_ERRTAG_IFLUSH_5,
- XFS_RANDOM_IFLUSH_5)) {
+ ip->i_d.di_nblocks, mp, XFS_ERRTAG_IFLUSH_5)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: detected corrupt incore inode %Lu, "
"total extents = %d, nblocks = %Ld, ptr 0x%p",
@@ -3529,7 +3528,7 @@ xfs_iflush_int(
goto corrupt_out;
}
if (XFS_TEST_ERROR(ip->i_d.di_forkoff > mp->m_sb.sb_inodesize,
- mp, XFS_ERRTAG_IFLUSH_6, XFS_RANDOM_IFLUSH_6)) {
+ mp, XFS_ERRTAG_IFLUSH_6)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: bad inode %Lu, forkoff 0x%x, ptr 0x%p",
__func__, ip->i_ino, ip->i_d.di_forkoff, ip);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 94e5bdf..304b79d 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -543,7 +543,7 @@ xfs_file_iomap_begin_delay(
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
- mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
+ mp, XFS_ERRTAG_BMAPIFORMAT))) {
XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
error = -EFSCORRUPTED;
goto out_unlock;
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 98b39cb..2d1112e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1189,8 +1189,7 @@ xlog_iodone(xfs_buf_t *bp)
* IOABORT state. The IOABORT state is only set in DEBUG mode to inject
* CRC errors into log recovery.
*/
- if (XFS_TEST_ERROR(bp->b_error, l->l_mp, XFS_ERRTAG_IODONE_IOERR,
- XFS_RANDOM_IODONE_IOERR) ||
+ if (XFS_TEST_ERROR(bp->b_error, l->l_mp, XFS_ERRTAG_IODONE_IOERR) ||
iclog->ic_state & XLOG_STATE_IOABORT) {
if (iclog->ic_state & XLOG_STATE_IOABORT)
iclog->ic_state &= ~XLOG_STATE_IOABORT;
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism
2017-06-21 1:11 [RFC PATCH 0/4] xfs: more configurable error injection Darrick J. Wong
` (2 preceding siblings ...)
2017-06-21 1:11 ` [PATCH 3/4] xfs: remove unneeded parameter from XFS_TEST_ERROR Darrick J. Wong
@ 2017-06-21 1:11 ` Darrick J. Wong
2017-06-21 18:19 ` Brian Foster
3 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 1:11 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
We now have enhanced error injection that can control the frequency
with which errors happen, so convert drop_writes to use this.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_error.c | 3 +++
fs/xfs/xfs_error.h | 14 +++++++++++++-
fs/xfs/xfs_iomap.c | 2 +-
fs/xfs/xfs_mount.h | 24 ------------------------
fs/xfs/xfs_sysfs.c | 42 ------------------------------------------
5 files changed, 17 insertions(+), 68 deletions(-)
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 81f260f..40a15e2 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -55,6 +55,7 @@ static unsigned int xfs_errortag_random_default[] = {
XFS_RANDOM_REFCOUNT_FINISH_ONE,
XFS_RANDOM_BMAP_FINISH_ONE,
XFS_RANDOM_AG_RESV_CRITICAL,
+ XFS_RANDOM_DROP_WRITES,
};
struct xfs_errortag_attr {
@@ -157,6 +158,7 @@ XFS_ERRORTAG_ATTR_RW(refcount_continue_update, XFS_ERRTAG_REFCOUNT_CONTINUE_UPDA
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);
+XFS_ERRORTAG_ATTR_RW(drop_writes, XFS_ERRTAG_DROP_WRITES);
static struct attribute *xfs_errortag_attrs[] = {
XFS_ERRORTAG_ATTR_LIST(noerror),
@@ -187,6 +189,7 @@ static struct attribute *xfs_errortag_attrs[] = {
XFS_ERRORTAG_ATTR_LIST(refcount_finish_one),
XFS_ERRORTAG_ATTR_LIST(bmap_finish_one),
XFS_ERRORTAG_ATTR_LIST(ag_resv_critical),
+ XFS_ERRORTAG_ATTR_LIST(drop_writes),
NULL,
};
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 083a6fc..cd7881f 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -96,7 +96,18 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
#define XFS_ERRTAG_REFCOUNT_FINISH_ONE 25
#define XFS_ERRTAG_BMAP_FINISH_ONE 26
#define XFS_ERRTAG_AG_RESV_CRITICAL 27
-#define XFS_ERRTAG_MAX 28
+
+/*
+ * DEBUG mode instrumentation to test and/or trigger delayed allocation
+ * block killing in the event of failed writes. When enabled, all
+ * buffered writes are silenty dropped and handled as if they failed.
+ * All delalloc blocks in the range of the write (including pre-existing
+ * delalloc blocks!) are tossed as part of the write failure error
+ * handling sequence.
+ */
+#define XFS_ERRTAG_DROP_WRITES 28
+
+#define XFS_ERRTAG_MAX 29
/*
* Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -129,6 +140,7 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
#define XFS_RANDOM_REFCOUNT_FINISH_ONE 1
#define XFS_RANDOM_BMAP_FINISH_ONE 1
#define XFS_RANDOM_AG_RESV_CRITICAL 4
+#define XFS_RANDOM_DROP_WRITES 1
#ifdef DEBUG
extern int xfs_errortag_init(struct xfs_mount *mp);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 304b79d..86f1a9f 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1097,7 +1097,7 @@ xfs_file_iomap_end_delalloc(
* Behave as if the write failed if drop writes is enabled. Set the NEW
* flag to force delalloc cleanup.
*/
- if (xfs_mp_drop_writes(mp)) {
+ if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
iomap->flags |= IOMAP_F_NEW;
written = 0;
}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 931e9fc..e0792d0 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -205,16 +205,6 @@ typedef struct xfs_mount {
*/
unsigned int *m_errortag;
struct xfs_kobj m_errortag_kobj;
-
- /*
- * DEBUG mode instrumentation to test and/or trigger delayed allocation
- * block killing in the event of failed writes. When enabled, all
- * buffered writes are silenty dropped and handled as if they failed.
- * All delalloc blocks in the range of the write (including pre-existing
- * delalloc blocks!) are tossed as part of the write failure error
- * handling sequence.
- */
- bool m_drop_writes;
#endif
} xfs_mount_t;
@@ -333,20 +323,6 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d)
return (xfs_agblock_t) do_div(ld, mp->m_sb.sb_agblocks);
}
-#ifdef DEBUG
-static inline bool
-xfs_mp_drop_writes(struct xfs_mount *mp)
-{
- return mp->m_drop_writes;
-}
-#else
-static inline bool
-xfs_mp_drop_writes(struct xfs_mount *mp)
-{
- return 0;
-}
-#endif
-
/* per-AG block reservation data structures*/
enum xfs_ag_resv_type {
XFS_AG_RESV_NONE = 0,
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index b86148a..3e52662 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -90,49 +90,7 @@ to_mp(struct kobject *kobject)
return container_of(kobj, struct xfs_mount, m_kobj);
}
-#ifdef DEBUG
-
-STATIC ssize_t
-drop_writes_store(
- struct kobject *kobject,
- const char *buf,
- size_t count)
-{
- struct xfs_mount *mp = to_mp(kobject);
- int ret;
- int val;
-
- ret = kstrtoint(buf, 0, &val);
- if (ret)
- return ret;
-
- if (val == 1)
- mp->m_drop_writes = true;
- else if (val == 0)
- mp->m_drop_writes = false;
- else
- return -EINVAL;
-
- return count;
-}
-
-STATIC ssize_t
-drop_writes_show(
- struct kobject *kobject,
- char *buf)
-{
- struct xfs_mount *mp = to_mp(kobject);
-
- return snprintf(buf, PAGE_SIZE, "%d\n", mp->m_drop_writes ? 1 : 0);
-}
-XFS_SYSFS_ATTR_RW(drop_writes);
-
-#endif /* DEBUG */
-
static struct attribute *xfs_mp_attrs[] = {
-#ifdef DEBUG
- ATTR_LIST(drop_writes),
-#endif
NULL,
};
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] xfs: make errortag a per-mountpoint structure
2017-06-21 1:11 ` [PATCH 1/4] xfs: make errortag a per-mountpoint structure Darrick J. Wong
@ 2017-06-21 18:18 ` Brian Foster
2017-06-21 18:46 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Brian Foster @ 2017-06-21 18:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Jun 20, 2017 at 06:11:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the xfs_etest structure in favor of a per-mountpoint structure.
> This will give us the flexibility to set as many error injection points
> as we want, and later enable us to set up sysfs knobs to set the trigger
> frequency as we wish. This comes at a cost of higher memory use, but
> unti we hit 1024 injection points (we're at 29) or a lot of mounts this
> shouldn't be a huge issue.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_error.c | 161 +++++++++++++++++++++++++++++++---------------------
> fs/xfs/xfs_error.h | 23 ++++---
> fs/xfs/xfs_ioctl.c | 4 +
> fs/xfs/xfs_mount.c | 10 +++
> fs/xfs/xfs_mount.h | 7 ++
> 5 files changed, 125 insertions(+), 80 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index ed7ee4e..ef16ac2 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -25,97 +25,126 @@
>
> #ifdef DEBUG
>
> -int xfs_etest[XFS_NUM_INJECT_ERROR];
> -int64_t xfs_etest_fsid[XFS_NUM_INJECT_ERROR];
> -char * xfs_etest_fsname[XFS_NUM_INJECT_ERROR];
> -int xfs_error_test_active;
> +static unsigned int xfs_errortag_random_default[] = {
> + XFS_RANDOM_DEFAULT,
> + XFS_RANDOM_IFLUSH_1,
> + XFS_RANDOM_IFLUSH_2,
> + XFS_RANDOM_IFLUSH_3,
> + XFS_RANDOM_IFLUSH_4,
> + XFS_RANDOM_IFLUSH_5,
> + XFS_RANDOM_IFLUSH_6,
> + XFS_RANDOM_DA_READ_BUF,
> + XFS_RANDOM_BTREE_CHECK_LBLOCK,
> + XFS_RANDOM_BTREE_CHECK_SBLOCK,
> + XFS_RANDOM_ALLOC_READ_AGF,
> + XFS_RANDOM_IALLOC_READ_AGI,
> + XFS_RANDOM_ITOBP_INOTOBP,
> + XFS_RANDOM_IUNLINK,
> + XFS_RANDOM_IUNLINK_REMOVE,
> + XFS_RANDOM_DIR_INO_VALIDATE,
> + XFS_RANDOM_BULKSTAT_READ_CHUNK,
> + XFS_RANDOM_IODONE_IOERR,
> + XFS_RANDOM_STRATREAD_IOERR,
> + XFS_RANDOM_STRATCMPL_IOERR,
> + XFS_RANDOM_DIOWRITE_IOERR,
> + XFS_RANDOM_BMAPIFORMAT,
> + XFS_RANDOM_FREE_EXTENT,
> + XFS_RANDOM_RMAP_FINISH_ONE,
> + XFS_RANDOM_REFCOUNT_CONTINUE_UPDATE,
> + XFS_RANDOM_REFCOUNT_FINISH_ONE,
> + XFS_RANDOM_BMAP_FINISH_ONE,
> + XFS_RANDOM_AG_RESV_CRITICAL,
> +};
>
> int
> -xfs_error_test(int error_tag, int *fsidp, char *expression,
> - int line, char *file, unsigned long randfactor)
> +xfs_errortag_init(
> + struct xfs_mount *mp)
> {
> - int i;
> - int64_t fsid;
> + mp->m_errortag = kmem_zalloc(sizeof(unsigned int) * XFS_ERRTAG_MAX,
> + KM_SLEEP | KM_MAYFAIL);
> + if (!mp->m_errortag)
> + return -ENOMEM;
> + return 0;
> +}
>
> - if (prandom_u32() % randfactor)
> - return 0;
> +void
> +xfs_errortag_del(
> + struct xfs_mount *mp)
> +{
> + kmem_free(mp->m_errortag);
> +}
>
> - memcpy(&fsid, fsidp, sizeof(xfs_fsid_t));
> +bool
> +xfs_errortag_test(
> + struct xfs_mount *mp,
> + const char *expression,
> + const char *file,
> + int line,
> + unsigned int error_tag)
> +{
> + unsigned int randfactor;
>
> - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> - if (xfs_etest[i] == error_tag && xfs_etest_fsid[i] == fsid) {
> - xfs_warn(NULL,
> - "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> - expression, file, line, xfs_etest_fsname[i]);
> - return 1;
> - }
> - }
> + ASSERT(error_tag < XFS_ERRTAG_MAX);
> + randfactor = mp->m_errortag[error_tag];
> + if (!randfactor || prandom_u32() % randfactor)
> + return false;
>
> - return 0;
> + xfs_warn(mp,
> +"Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> + expression, file, line, mp->m_fsname);
> + return true;
I wonder if we're going to want/need to ratelimit this now that we have
dynamic frequency support. I'm thinking cases like 100% error frequency
for the purpose of AIL item pinning could generate quite a bit of
output.
> }
>
> int
> -xfs_errortag_add(unsigned int error_tag, xfs_mount_t *mp)
> +xfs_errortag_set(
> + struct xfs_mount *mp,
> + unsigned int error_tag,
> + unsigned int tag_value)
> {
> - int i;
> - int len;
> - int64_t fsid;
> -
> if (error_tag >= XFS_ERRTAG_MAX)
> return -EINVAL;
>
> - memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
> + mp->m_errortag[error_tag] = tag_value;
> + xfs_warn(mp,
> +"Turned %s XFS error tag #%d",
> + tag_value ? "on" : "off", error_tag);
> + return 0;
This is unnecessarily noisy IMO as well. If we wanted to preserve it for
the ioctl() case, we could pull it up into the caller. Unless it's
actually used somewhere that I'm not aware of, I'd be fine with killing
it altogether.
> +}
>
> - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> - if (xfs_etest_fsid[i] == fsid && xfs_etest[i] == error_tag) {
> - xfs_warn(mp, "error tag #%d on", error_tag);
> - return 0;
> - }
> - }
> +int
> +xfs_errortag_add(
> + struct xfs_mount *mp,
> + unsigned int error_tag)
> +{
> + if (error_tag >= XFS_ERRTAG_MAX)
> + return -EINVAL;
>
> - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> - if (xfs_etest[i] == 0) {
> - xfs_warn(mp, "Turned on XFS error tag #%d",
> - error_tag);
> - xfs_etest[i] = error_tag;
> - xfs_etest_fsid[i] = fsid;
> - len = strlen(mp->m_fsname);
> - xfs_etest_fsname[i] = kmem_alloc(len + 1, KM_SLEEP);
> - strcpy(xfs_etest_fsname[i], mp->m_fsname);
> - xfs_error_test_active++;
> - return 0;
> - }
> + if (mp->m_errortag[error_tag]) {
> + xfs_warn(mp, "error tag #%d on", error_tag);
> + return 0;
It probably doesn't matter that much, but I wonder if we should still
set the _random_default value even if the tag is already enabled. The
idea being to (sort of) preserve backwards compatibility for the ioctl()
in the event that the tag was already enabled via sysfs.
Now that I think of it, does the ioctl() interface even support turning
off an individual error tag? :/
Brian
> }
>
> - xfs_warn(mp, "error tag overflow, too many turned on");
> -
> - return 1;
> + return xfs_errortag_set(mp, error_tag,
> + xfs_errortag_random_default[error_tag]);
> }
>
> int
> -xfs_errortag_clearall(xfs_mount_t *mp, int loud)
> +xfs_errortag_clearall(
> + struct xfs_mount *mp,
> + bool loud)
> {
> - int64_t fsid;
> - int cleared = 0;
> - int i;
> -
> - memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
> -
> -
> - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> - if ((fsid == 0LL || xfs_etest_fsid[i] == fsid) &&
> - xfs_etest[i] != 0) {
> - cleared = 1;
> - xfs_warn(mp, "Clearing XFS error tag #%d",
> - xfs_etest[i]);
> - xfs_etest[i] = 0;
> - xfs_etest_fsid[i] = 0LL;
> - kmem_free(xfs_etest_fsname[i]);
> - xfs_etest_fsname[i] = NULL;
> - xfs_error_test_active--;
> + bool cleared = false;
> + unsigned int i;
> +
> + for (i = 0; i < XFS_ERRTAG_MAX; i++) {
> + if (mp->m_errortag[i]) {
> + cleared = true;
> + break;
> }
> }
>
> + memset(mp->m_errortag, 0, sizeof(unsigned int) * XFS_ERRTAG_MAX);
> +
> if (loud || cleared)
> xfs_warn(mp, "Cleared all XFS error tags for filesystem");
>
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index 05f8666..341e8a0 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -131,20 +131,23 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> #define XFS_RANDOM_AG_RESV_CRITICAL 4
>
> #ifdef DEBUG
> -extern int xfs_error_test_active;
> -extern int xfs_error_test(int, int *, char *, int, char *, unsigned long);
> -
> -#define XFS_NUM_INJECT_ERROR 10
> +extern int xfs_errortag_init(struct xfs_mount *mp);
> +extern void xfs_errortag_del(struct xfs_mount *mp);
> +extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
> + const char *file, int line, unsigned int error_tag);
> #define XFS_TEST_ERROR(expr, mp, tag, rf) \
> - ((expr) || (xfs_error_test_active && \
> - xfs_error_test((tag), (mp)->m_fixedfsid, "expr", __LINE__, __FILE__, \
> - (rf))))
> + ((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
>
> -extern int xfs_errortag_add(unsigned int error_tag, struct xfs_mount *mp);
> -extern int xfs_errortag_clearall(struct xfs_mount *mp, int loud);
> +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);
> +extern int xfs_errortag_clearall(struct xfs_mount *mp, bool loud);
> #else
> +#define xfs_errortag_init(mp) (0)
> +#define xfs_errortag_del(mp)
> #define XFS_TEST_ERROR(expr, mp, tag, rf) (expr)
> -#define xfs_errortag_add(tag, mp) (ENOSYS)
> +#define xfs_errortag_set(mp, tag, val) (ENOSYS)
> +#define xfs_errortag_add(mp, tag) (ENOSYS)
> #define xfs_errortag_clearall(mp, loud) (ENOSYS)
> #endif /* DEBUG */
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 8ffe4ea..ab0894a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -2037,14 +2037,14 @@ xfs_file_ioctl(
> if (copy_from_user(&in, arg, sizeof(in)))
> return -EFAULT;
>
> - return xfs_errortag_add(in.errtag, mp);
> + return xfs_errortag_add(mp, in.errtag);
> }
>
> case XFS_IOC_ERROR_CLEARALL:
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> - return xfs_errortag_clearall(mp, 1);
> + return xfs_errortag_clearall(mp, true);
>
> case XFS_IOC_FREE_EOFBLOCKS: {
> struct xfs_fs_eofblocks eofb;
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index cc6789d..5cdcf43 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -720,10 +720,13 @@ xfs_mountfs(
> if (error)
> goto out_del_stats;
>
> + error = xfs_errortag_init(mp);
> + if (error)
> + goto out_remove_error_sysfs;
>
> error = xfs_uuid_mount(mp);
> if (error)
> - goto out_remove_error_sysfs;
> + goto out_remove_errortag;
>
> /*
> * Set the minimum read and write sizes
> @@ -1042,6 +1045,8 @@ xfs_mountfs(
> xfs_da_unmount(mp);
> out_remove_uuid:
> xfs_uuid_unmount(mp);
> + out_remove_errortag:
> + xfs_errortag_del(mp);
> out_remove_error_sysfs:
> xfs_error_sysfs_del(mp);
> out_del_stats:
> @@ -1145,10 +1150,11 @@ xfs_unmountfs(
> xfs_uuid_unmount(mp);
>
> #if defined(DEBUG)
> - xfs_errortag_clearall(mp, 0);
> + xfs_errortag_clearall(mp, false);
> #endif
> xfs_free_perag(mp);
>
> + xfs_errortag_del(mp);
> xfs_error_sysfs_del(mp);
> xfs_sysfs_del(&mp->m_stats.xs_kobj);
> xfs_sysfs_del(&mp->m_kobj);
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 305d953..e002ac5 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -199,6 +199,13 @@ typedef struct xfs_mount {
> bool m_fail_unmount;
> #ifdef DEBUG
> /*
> + * Frequency with which errors are injected. Replaces xfs_etest; the
> + * value stored in here is the inverse of the frequency with which the
> + * error triggers. 1 = always, 2 = half the time, etc.
> + */
> + unsigned int *m_errortag;
> +
> + /*
> * DEBUG mode instrumentation to test and/or trigger delayed allocation
> * block killing in the event of failed writes. When enabled, all
> * buffered writes are silenty dropped and handled as if they failed.
>
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-21 1:11 ` [PATCH 2/4] xfs: expose errortag knobs via sysfs Darrick J. Wong
@ 2017-06-21 18:19 ` Brian Foster
2017-06-21 18:39 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Brian Foster @ 2017-06-21 18:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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'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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] xfs: remove unneeded parameter from XFS_TEST_ERROR
2017-06-21 1:11 ` [PATCH 3/4] xfs: remove unneeded parameter from XFS_TEST_ERROR Darrick J. Wong
@ 2017-06-21 18:19 ` Brian Foster
0 siblings, 0 replies; 20+ messages in thread
From: Brian Foster @ 2017-06-21 18:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Jun 20, 2017 at 06:11:41PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Since we moved the injected error frequency controls to the mountpoint,
> we can get rid of the last argument to XFS_TEST_ERROR.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Make sense...
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_ag_resv.c | 3 +--
> fs/xfs/libxfs/xfs_alloc.c | 6 ++----
> fs/xfs/libxfs/xfs_bmap.c | 13 ++++++-------
> fs/xfs/libxfs/xfs_btree.c | 6 ++----
> fs/xfs/libxfs/xfs_dir2.c | 3 +--
> fs/xfs/libxfs/xfs_ialloc.c | 3 +--
> fs/xfs/libxfs/xfs_inode_buf.c | 3 +--
> fs/xfs/libxfs/xfs_refcount.c | 6 ++----
> fs/xfs/libxfs/xfs_rmap.c | 3 +--
> fs/xfs/xfs_error.h | 4 ++--
> fs/xfs/xfs_inode.c | 11 +++++------
> fs/xfs/xfs_iomap.c | 2 +-
> fs/xfs/xfs_log.c | 3 +--
> 13 files changed, 26 insertions(+), 40 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 33db69b..b008ff3 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -111,8 +111,7 @@ xfs_ag_resv_critical(
>
> /* Critically low if less than 10% or max btree height remains. */
> return XFS_TEST_ERROR(avail < orig / 10 || avail < XFS_BTREE_MAXLEVELS,
> - pag->pag_mount, XFS_ERRTAG_AG_RESV_CRITICAL,
> - XFS_RANDOM_AG_RESV_CRITICAL);
> + pag->pag_mount, XFS_ERRTAG_AG_RESV_CRITICAL);
> }
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index fefa8da..744dcae 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2454,8 +2454,7 @@ xfs_agf_read_verify(
> !xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF))
> xfs_buf_ioerror(bp, -EFSBADCRC);
> else if (XFS_TEST_ERROR(!xfs_agf_verify(mp, bp), mp,
> - XFS_ERRTAG_ALLOC_READ_AGF,
> - XFS_RANDOM_ALLOC_READ_AGF))
> + XFS_ERRTAG_ALLOC_READ_AGF))
> xfs_buf_ioerror(bp, -EFSCORRUPTED);
>
> if (bp->b_error)
> @@ -2842,8 +2841,7 @@ xfs_free_extent(
> ASSERT(type != XFS_AG_RESV_AGFL);
>
> if (XFS_TEST_ERROR(false, mp,
> - XFS_ERRTAG_FREE_EXTENT,
> - XFS_RANDOM_FREE_EXTENT))
> + XFS_ERRTAG_FREE_EXTENT))
> return -EIO;
>
> error = xfs_free_extent_fix_freelist(tp, agno, &agbp);
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 19480ed..0a98807 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3992,7 +3992,7 @@ xfs_bmapi_read(
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT("xfs_bmapi_read", XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> }
> @@ -4473,7 +4473,7 @@ xfs_bmapi_write(
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT("xfs_bmapi_write", XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> }
> @@ -4694,7 +4694,7 @@ xfs_bmapi_remap(
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> }
> @@ -6098,7 +6098,7 @@ xfs_bmap_shift_extents(
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT("xfs_bmap_shift_extents",
> XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> @@ -6250,7 +6250,7 @@ xfs_bmap_split_extent_at(
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT("xfs_bmap_split_extent_at",
> XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> @@ -6508,8 +6508,7 @@ xfs_bmap_finish_one(
> return -EFSCORRUPTED;
>
> if (XFS_TEST_ERROR(false, tp->t_mountp,
> - XFS_ERRTAG_BMAP_FINISH_ONE,
> - XFS_RANDOM_BMAP_FINISH_ONE))
> + XFS_ERRTAG_BMAP_FINISH_ONE))
> return -EIO;
>
> switch (type) {
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 2f8075a..4da85ff 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -101,8 +101,7 @@ xfs_btree_check_lblock(
> be64_to_cpu(block->bb_u.l.bb_rightsib)));
>
> if (unlikely(XFS_TEST_ERROR(!lblock_ok, mp,
> - XFS_ERRTAG_BTREE_CHECK_LBLOCK,
> - XFS_RANDOM_BTREE_CHECK_LBLOCK))) {
> + XFS_ERRTAG_BTREE_CHECK_LBLOCK))) {
> if (bp)
> trace_xfs_btree_corrupt(bp, _RET_IP_);
> XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> @@ -153,8 +152,7 @@ xfs_btree_check_sblock(
> block->bb_u.s.bb_rightsib;
>
> if (unlikely(XFS_TEST_ERROR(!sblock_ok, mp,
> - XFS_ERRTAG_BTREE_CHECK_SBLOCK,
> - XFS_RANDOM_BTREE_CHECK_SBLOCK))) {
> + XFS_ERRTAG_BTREE_CHECK_SBLOCK))) {
> if (bp)
> trace_xfs_btree_corrupt(bp, _RET_IP_);
> XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 2f389d36..ccf9783 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -218,8 +218,7 @@ xfs_dir_ino_validate(
> agblkno != 0 &&
> ioff < (1 << mp->m_sb.sb_inopblog) &&
> XFS_AGINO_TO_INO(mp, agno, agino) == ino;
> - if (unlikely(XFS_TEST_ERROR(!ino_ok, mp, XFS_ERRTAG_DIR_INO_VALIDATE,
> - XFS_RANDOM_DIR_INO_VALIDATE))) {
> + if (unlikely(XFS_TEST_ERROR(!ino_ok, mp, XFS_ERRTAG_DIR_INO_VALIDATE))) {
> xfs_warn(mp, "Invalid inode number 0x%Lx",
> (unsigned long long) ino);
> XFS_ERROR_REPORT("xfs_dir_ino_validate", XFS_ERRLEVEL_LOW, mp);
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index c514fe9..ffd5a15 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -2552,8 +2552,7 @@ xfs_agi_read_verify(
> !xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF))
> xfs_buf_ioerror(bp, -EFSBADCRC);
> else if (XFS_TEST_ERROR(!xfs_agi_verify(bp), mp,
> - XFS_ERRTAG_IALLOC_READ_AGI,
> - XFS_RANDOM_IALLOC_READ_AGI))
> + XFS_ERRTAG_IALLOC_READ_AGI))
> xfs_buf_ioerror(bp, -EFSCORRUPTED);
>
> if (bp->b_error)
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 0c970cf..378f8fb 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -105,8 +105,7 @@ xfs_inode_buf_verify(
> di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
> xfs_dinode_good_version(mp, dip->di_version);
> if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
> - XFS_ERRTAG_ITOBP_INOTOBP,
> - XFS_RANDOM_ITOBP_INOTOBP))) {
> + XFS_ERRTAG_ITOBP_INOTOBP))) {
> if (readahead) {
> bp->b_flags &= ~XBF_DONE;
> xfs_buf_ioerror(bp, -EIO);
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index e170161..900ea23 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -805,8 +805,7 @@ xfs_refcount_still_have_space(
> */
> if (cur->bc_private.a.priv.refc.nr_ops > 2 &&
> XFS_TEST_ERROR(false, cur->bc_mp,
> - XFS_ERRTAG_REFCOUNT_CONTINUE_UPDATE,
> - XFS_RANDOM_REFCOUNT_CONTINUE_UPDATE))
> + XFS_ERRTAG_REFCOUNT_CONTINUE_UPDATE))
> return false;
>
> if (cur->bc_private.a.priv.refc.nr_ops == 0)
> @@ -1068,8 +1067,7 @@ xfs_refcount_finish_one(
> blockcount);
>
> if (XFS_TEST_ERROR(false, mp,
> - XFS_ERRTAG_REFCOUNT_FINISH_ONE,
> - XFS_RANDOM_REFCOUNT_FINISH_ONE))
> + XFS_ERRTAG_REFCOUNT_FINISH_ONE))
> return -EIO;
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> index eda275b..55c88a7 100644
> --- a/fs/xfs/libxfs/xfs_rmap.c
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -2087,8 +2087,7 @@ xfs_rmap_finish_one(
> startoff, blockcount, state);
>
> if (XFS_TEST_ERROR(false, mp,
> - XFS_ERRTAG_RMAP_FINISH_ONE,
> - XFS_RANDOM_RMAP_FINISH_ONE))
> + XFS_ERRTAG_RMAP_FINISH_ONE))
> return -EIO;
>
> /*
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index a3c0c1d..083a6fc 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -135,7 +135,7 @@ extern int xfs_errortag_init(struct xfs_mount *mp);
> extern void xfs_errortag_del(struct xfs_mount *mp);
> extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
> const char *file, int line, unsigned int error_tag);
> -#define XFS_TEST_ERROR(expr, mp, tag, rf) \
> +#define XFS_TEST_ERROR(expr, mp, tag) \
> ((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
>
> extern int xfs_errortag_get(struct xfs_mount *mp, unsigned int error_tag);
> @@ -146,7 +146,7 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp, bool loud);
> #else
> #define xfs_errortag_init(mp) (0)
> #define xfs_errortag_del(mp)
> -#define XFS_TEST_ERROR(expr, mp, tag, rf) (expr)
> +#define XFS_TEST_ERROR(expr, mp, tag) (expr)
> #define xfs_errortag_set(mp, tag, val) (ENOSYS)
> #define xfs_errortag_add(mp, tag) (ENOSYS)
> #define xfs_errortag_clearall(mp, loud) (ENOSYS)
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ffbfe7d..67bd563 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3489,7 +3489,7 @@ xfs_iflush_int(
> dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
>
> if (XFS_TEST_ERROR(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC),
> - mp, XFS_ERRTAG_IFLUSH_1, XFS_RANDOM_IFLUSH_1)) {
> + mp, XFS_ERRTAG_IFLUSH_1)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad inode %Lu magic number 0x%x, ptr 0x%p",
> __func__, ip->i_ino, be16_to_cpu(dip->di_magic), dip);
> @@ -3499,7 +3499,7 @@ xfs_iflush_int(
> if (XFS_TEST_ERROR(
> (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
> (ip->i_d.di_format != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_IFLUSH_3, XFS_RANDOM_IFLUSH_3)) {
> + mp, XFS_ERRTAG_IFLUSH_3)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad regular inode %Lu, ptr 0x%p",
> __func__, ip->i_ino, ip);
> @@ -3510,7 +3510,7 @@ xfs_iflush_int(
> (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
> (ip->i_d.di_format != XFS_DINODE_FMT_BTREE) &&
> (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL),
> - mp, XFS_ERRTAG_IFLUSH_4, XFS_RANDOM_IFLUSH_4)) {
> + mp, XFS_ERRTAG_IFLUSH_4)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad directory inode %Lu, ptr 0x%p",
> __func__, ip->i_ino, ip);
> @@ -3518,8 +3518,7 @@ xfs_iflush_int(
> }
> }
> if (XFS_TEST_ERROR(ip->i_d.di_nextents + ip->i_d.di_anextents >
> - ip->i_d.di_nblocks, mp, XFS_ERRTAG_IFLUSH_5,
> - XFS_RANDOM_IFLUSH_5)) {
> + ip->i_d.di_nblocks, mp, XFS_ERRTAG_IFLUSH_5)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: detected corrupt incore inode %Lu, "
> "total extents = %d, nblocks = %Ld, ptr 0x%p",
> @@ -3529,7 +3528,7 @@ xfs_iflush_int(
> goto corrupt_out;
> }
> if (XFS_TEST_ERROR(ip->i_d.di_forkoff > mp->m_sb.sb_inodesize,
> - mp, XFS_ERRTAG_IFLUSH_6, XFS_RANDOM_IFLUSH_6)) {
> + mp, XFS_ERRTAG_IFLUSH_6)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: bad inode %Lu, forkoff 0x%x, ptr 0x%p",
> __func__, ip->i_ino, ip->i_d.di_forkoff, ip);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 94e5bdf..304b79d 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -543,7 +543,7 @@ xfs_file_iomap_begin_delay(
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> - mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> error = -EFSCORRUPTED;
> goto out_unlock;
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 98b39cb..2d1112e 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1189,8 +1189,7 @@ xlog_iodone(xfs_buf_t *bp)
> * IOABORT state. The IOABORT state is only set in DEBUG mode to inject
> * CRC errors into log recovery.
> */
> - if (XFS_TEST_ERROR(bp->b_error, l->l_mp, XFS_ERRTAG_IODONE_IOERR,
> - XFS_RANDOM_IODONE_IOERR) ||
> + if (XFS_TEST_ERROR(bp->b_error, l->l_mp, XFS_ERRTAG_IODONE_IOERR) ||
> iclog->ic_state & XLOG_STATE_IOABORT) {
> if (iclog->ic_state & XLOG_STATE_IOABORT)
> iclog->ic_state &= ~XLOG_STATE_IOABORT;
>
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism
2017-06-21 1:11 ` [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism Darrick J. Wong
@ 2017-06-21 18:19 ` Brian Foster
0 siblings, 0 replies; 20+ messages in thread
From: Brian Foster @ 2017-06-21 18:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Jun 20, 2017 at 06:11:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> We now have enhanced error injection that can control the frequency
> with which errors happen, so convert drop_writes to use this.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Nice. Looks like a simple tweak to xfs/196 too...
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_error.c | 3 +++
> fs/xfs/xfs_error.h | 14 +++++++++++++-
> fs/xfs/xfs_iomap.c | 2 +-
> fs/xfs/xfs_mount.h | 24 ------------------------
> fs/xfs/xfs_sysfs.c | 42 ------------------------------------------
> 5 files changed, 17 insertions(+), 68 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 81f260f..40a15e2 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -55,6 +55,7 @@ static unsigned int xfs_errortag_random_default[] = {
> XFS_RANDOM_REFCOUNT_FINISH_ONE,
> XFS_RANDOM_BMAP_FINISH_ONE,
> XFS_RANDOM_AG_RESV_CRITICAL,
> + XFS_RANDOM_DROP_WRITES,
> };
>
> struct xfs_errortag_attr {
> @@ -157,6 +158,7 @@ XFS_ERRORTAG_ATTR_RW(refcount_continue_update, XFS_ERRTAG_REFCOUNT_CONTINUE_UPDA
> 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);
> +XFS_ERRORTAG_ATTR_RW(drop_writes, XFS_ERRTAG_DROP_WRITES);
>
> static struct attribute *xfs_errortag_attrs[] = {
> XFS_ERRORTAG_ATTR_LIST(noerror),
> @@ -187,6 +189,7 @@ static struct attribute *xfs_errortag_attrs[] = {
> XFS_ERRORTAG_ATTR_LIST(refcount_finish_one),
> XFS_ERRORTAG_ATTR_LIST(bmap_finish_one),
> XFS_ERRORTAG_ATTR_LIST(ag_resv_critical),
> + XFS_ERRORTAG_ATTR_LIST(drop_writes),
> NULL,
> };
>
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index 083a6fc..cd7881f 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -96,7 +96,18 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> #define XFS_ERRTAG_REFCOUNT_FINISH_ONE 25
> #define XFS_ERRTAG_BMAP_FINISH_ONE 26
> #define XFS_ERRTAG_AG_RESV_CRITICAL 27
> -#define XFS_ERRTAG_MAX 28
> +
> +/*
> + * DEBUG mode instrumentation to test and/or trigger delayed allocation
> + * block killing in the event of failed writes. When enabled, all
> + * buffered writes are silenty dropped and handled as if they failed.
> + * All delalloc blocks in the range of the write (including pre-existing
> + * delalloc blocks!) are tossed as part of the write failure error
> + * handling sequence.
> + */
> +#define XFS_ERRTAG_DROP_WRITES 28
> +
> +#define XFS_ERRTAG_MAX 29
>
> /*
> * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
> @@ -129,6 +140,7 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> #define XFS_RANDOM_REFCOUNT_FINISH_ONE 1
> #define XFS_RANDOM_BMAP_FINISH_ONE 1
> #define XFS_RANDOM_AG_RESV_CRITICAL 4
> +#define XFS_RANDOM_DROP_WRITES 1
>
> #ifdef DEBUG
> extern int xfs_errortag_init(struct xfs_mount *mp);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 304b79d..86f1a9f 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1097,7 +1097,7 @@ xfs_file_iomap_end_delalloc(
> * Behave as if the write failed if drop writes is enabled. Set the NEW
> * flag to force delalloc cleanup.
> */
> - if (xfs_mp_drop_writes(mp)) {
> + if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
> iomap->flags |= IOMAP_F_NEW;
> written = 0;
> }
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 931e9fc..e0792d0 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -205,16 +205,6 @@ typedef struct xfs_mount {
> */
> unsigned int *m_errortag;
> struct xfs_kobj m_errortag_kobj;
> -
> - /*
> - * DEBUG mode instrumentation to test and/or trigger delayed allocation
> - * block killing in the event of failed writes. When enabled, all
> - * buffered writes are silenty dropped and handled as if they failed.
> - * All delalloc blocks in the range of the write (including pre-existing
> - * delalloc blocks!) are tossed as part of the write failure error
> - * handling sequence.
> - */
> - bool m_drop_writes;
> #endif
> } xfs_mount_t;
>
> @@ -333,20 +323,6 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d)
> return (xfs_agblock_t) do_div(ld, mp->m_sb.sb_agblocks);
> }
>
> -#ifdef DEBUG
> -static inline bool
> -xfs_mp_drop_writes(struct xfs_mount *mp)
> -{
> - return mp->m_drop_writes;
> -}
> -#else
> -static inline bool
> -xfs_mp_drop_writes(struct xfs_mount *mp)
> -{
> - return 0;
> -}
> -#endif
> -
> /* per-AG block reservation data structures*/
> enum xfs_ag_resv_type {
> XFS_AG_RESV_NONE = 0,
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index b86148a..3e52662 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -90,49 +90,7 @@ to_mp(struct kobject *kobject)
> return container_of(kobj, struct xfs_mount, m_kobj);
> }
>
> -#ifdef DEBUG
> -
> -STATIC ssize_t
> -drop_writes_store(
> - struct kobject *kobject,
> - const char *buf,
> - size_t count)
> -{
> - struct xfs_mount *mp = to_mp(kobject);
> - int ret;
> - int val;
> -
> - ret = kstrtoint(buf, 0, &val);
> - if (ret)
> - return ret;
> -
> - if (val == 1)
> - mp->m_drop_writes = true;
> - else if (val == 0)
> - mp->m_drop_writes = false;
> - else
> - return -EINVAL;
> -
> - return count;
> -}
> -
> -STATIC ssize_t
> -drop_writes_show(
> - struct kobject *kobject,
> - char *buf)
> -{
> - struct xfs_mount *mp = to_mp(kobject);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n", mp->m_drop_writes ? 1 : 0);
> -}
> -XFS_SYSFS_ATTR_RW(drop_writes);
> -
> -#endif /* DEBUG */
> -
> static struct attribute *xfs_mp_attrs[] = {
> -#ifdef DEBUG
> - ATTR_LIST(drop_writes),
> -#endif
> NULL,
> };
>
>
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-21 18:19 ` Brian Foster
@ 2017-06-21 18:39 ` Darrick J. Wong
2017-06-21 18:53 ` Brian Foster
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 18:39 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] xfs: make errortag a per-mountpoint structure
2017-06-21 18:18 ` Brian Foster
@ 2017-06-21 18:46 ` Darrick J. Wong
2017-06-21 19:05 ` Brian Foster
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 18:46 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Wed, Jun 21, 2017 at 02:18:48PM -0400, Brian Foster wrote:
> On Tue, Jun 20, 2017 at 06:11:18PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Remove the xfs_etest structure in favor of a per-mountpoint structure.
> > This will give us the flexibility to set as many error injection points
> > as we want, and later enable us to set up sysfs knobs to set the trigger
> > frequency as we wish. This comes at a cost of higher memory use, but
> > unti we hit 1024 injection points (we're at 29) or a lot of mounts this
> > shouldn't be a huge issue.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/xfs_error.c | 161 +++++++++++++++++++++++++++++++---------------------
> > fs/xfs/xfs_error.h | 23 ++++---
> > fs/xfs/xfs_ioctl.c | 4 +
> > fs/xfs/xfs_mount.c | 10 +++
> > fs/xfs/xfs_mount.h | 7 ++
> > 5 files changed, 125 insertions(+), 80 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > index ed7ee4e..ef16ac2 100644
> > --- a/fs/xfs/xfs_error.c
> > +++ b/fs/xfs/xfs_error.c
> > @@ -25,97 +25,126 @@
> >
> > #ifdef DEBUG
> >
> > -int xfs_etest[XFS_NUM_INJECT_ERROR];
> > -int64_t xfs_etest_fsid[XFS_NUM_INJECT_ERROR];
> > -char * xfs_etest_fsname[XFS_NUM_INJECT_ERROR];
> > -int xfs_error_test_active;
> > +static unsigned int xfs_errortag_random_default[] = {
> > + XFS_RANDOM_DEFAULT,
> > + XFS_RANDOM_IFLUSH_1,
> > + XFS_RANDOM_IFLUSH_2,
> > + XFS_RANDOM_IFLUSH_3,
> > + XFS_RANDOM_IFLUSH_4,
> > + XFS_RANDOM_IFLUSH_5,
> > + XFS_RANDOM_IFLUSH_6,
> > + XFS_RANDOM_DA_READ_BUF,
> > + XFS_RANDOM_BTREE_CHECK_LBLOCK,
> > + XFS_RANDOM_BTREE_CHECK_SBLOCK,
> > + XFS_RANDOM_ALLOC_READ_AGF,
> > + XFS_RANDOM_IALLOC_READ_AGI,
> > + XFS_RANDOM_ITOBP_INOTOBP,
> > + XFS_RANDOM_IUNLINK,
> > + XFS_RANDOM_IUNLINK_REMOVE,
> > + XFS_RANDOM_DIR_INO_VALIDATE,
> > + XFS_RANDOM_BULKSTAT_READ_CHUNK,
> > + XFS_RANDOM_IODONE_IOERR,
> > + XFS_RANDOM_STRATREAD_IOERR,
> > + XFS_RANDOM_STRATCMPL_IOERR,
> > + XFS_RANDOM_DIOWRITE_IOERR,
> > + XFS_RANDOM_BMAPIFORMAT,
> > + XFS_RANDOM_FREE_EXTENT,
> > + XFS_RANDOM_RMAP_FINISH_ONE,
> > + XFS_RANDOM_REFCOUNT_CONTINUE_UPDATE,
> > + XFS_RANDOM_REFCOUNT_FINISH_ONE,
> > + XFS_RANDOM_BMAP_FINISH_ONE,
> > + XFS_RANDOM_AG_RESV_CRITICAL,
> > +};
> >
> > int
> > -xfs_error_test(int error_tag, int *fsidp, char *expression,
> > - int line, char *file, unsigned long randfactor)
> > +xfs_errortag_init(
> > + struct xfs_mount *mp)
> > {
> > - int i;
> > - int64_t fsid;
> > + mp->m_errortag = kmem_zalloc(sizeof(unsigned int) * XFS_ERRTAG_MAX,
> > + KM_SLEEP | KM_MAYFAIL);
> > + if (!mp->m_errortag)
> > + return -ENOMEM;
> > + return 0;
> > +}
> >
> > - if (prandom_u32() % randfactor)
> > - return 0;
> > +void
> > +xfs_errortag_del(
> > + struct xfs_mount *mp)
> > +{
> > + kmem_free(mp->m_errortag);
> > +}
> >
> > - memcpy(&fsid, fsidp, sizeof(xfs_fsid_t));
> > +bool
> > +xfs_errortag_test(
> > + struct xfs_mount *mp,
> > + const char *expression,
> > + const char *file,
> > + int line,
> > + unsigned int error_tag)
> > +{
> > + unsigned int randfactor;
> >
> > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > - if (xfs_etest[i] == error_tag && xfs_etest_fsid[i] == fsid) {
> > - xfs_warn(NULL,
> > - "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> > - expression, file, line, xfs_etest_fsname[i]);
> > - return 1;
> > - }
> > - }
> > + ASSERT(error_tag < XFS_ERRTAG_MAX);
> > + randfactor = mp->m_errortag[error_tag];
> > + if (!randfactor || prandom_u32() % randfactor)
> > + return false;
> >
> > - return 0;
> > + xfs_warn(mp,
> > +"Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> > + expression, file, line, mp->m_fsname);
> > + return true;
>
> I wonder if we're going to want/need to ratelimit this now that we have
> dynamic frequency support. I'm thinking cases like 100% error frequency
> for the purpose of AIL item pinning could generate quite a bit of
> output.
Yeah, we should xfs_warn_ratelimited, the one I made to force repair
to repair undamaged metadata is particularly noisy.
>
> > }
> >
> > int
> > -xfs_errortag_add(unsigned int error_tag, xfs_mount_t *mp)
> > +xfs_errortag_set(
> > + struct xfs_mount *mp,
> > + unsigned int error_tag,
> > + unsigned int tag_value)
> > {
> > - int i;
> > - int len;
> > - int64_t fsid;
> > -
> > if (error_tag >= XFS_ERRTAG_MAX)
> > return -EINVAL;
> >
> > - memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
> > + mp->m_errortag[error_tag] = tag_value;
> > + xfs_warn(mp,
> > +"Turned %s XFS error tag #%d",
> > + tag_value ? "on" : "off", error_tag);
> > + return 0;
>
> This is unnecessarily noisy IMO as well. If we wanted to preserve it for
> the ioctl() case, we could pull it up into the caller. Unless it's
> actually used somewhere that I'm not aware of, I'd be fine with killing
> it altogether.
Sure. We cough out an error message every time we actually inject
an error anyway.
>
> > +}
> >
> > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > - if (xfs_etest_fsid[i] == fsid && xfs_etest[i] == error_tag) {
> > - xfs_warn(mp, "error tag #%d on", error_tag);
> > - return 0;
> > - }
> > - }
> > +int
> > +xfs_errortag_add(
> > + struct xfs_mount *mp,
> > + unsigned int error_tag)
> > +{
> > + if (error_tag >= XFS_ERRTAG_MAX)
> > + return -EINVAL;
> >
> > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > - if (xfs_etest[i] == 0) {
> > - xfs_warn(mp, "Turned on XFS error tag #%d",
> > - error_tag);
> > - xfs_etest[i] = error_tag;
> > - xfs_etest_fsid[i] = fsid;
> > - len = strlen(mp->m_fsname);
> > - xfs_etest_fsname[i] = kmem_alloc(len + 1, KM_SLEEP);
> > - strcpy(xfs_etest_fsname[i], mp->m_fsname);
> > - xfs_error_test_active++;
> > - return 0;
> > - }
> > + if (mp->m_errortag[error_tag]) {
> > + xfs_warn(mp, "error tag #%d on", error_tag);
> > + return 0;
>
> It probably doesn't matter that much, but I wonder if we should still
> set the _random_default value even if the tag is already enabled. The
The ioctl errored out if the tag was already enabled, so this preserves
that behavior. Note that xfs_errortag_add is only called by the ioctl,
whereas xfs_errortag_set is called by sysfs and it doesn't care what
the previous value was.
Granted, maybe we don't care about checking if it's already set and
should just go ahead and change the value, but that would change the
behavior of an existing (debug) interface.
> idea being to (sort of) preserve backwards compatibility for the ioctl()
> in the event that the tag was already enabled via sysfs.
>
> Now that I think of it, does the ioctl() interface even support turning
> off an individual error tag? :/
Correct. You only can clear all of them, at least via ioctls.
--D
>
> Brian
>
> > }
> >
> > - xfs_warn(mp, "error tag overflow, too many turned on");
> > -
> > - return 1;
> > + return xfs_errortag_set(mp, error_tag,
> > + xfs_errortag_random_default[error_tag]);
> > }
> >
> > int
> > -xfs_errortag_clearall(xfs_mount_t *mp, int loud)
> > +xfs_errortag_clearall(
> > + struct xfs_mount *mp,
> > + bool loud)
> > {
> > - int64_t fsid;
> > - int cleared = 0;
> > - int i;
> > -
> > - memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
> > -
> > -
> > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > - if ((fsid == 0LL || xfs_etest_fsid[i] == fsid) &&
> > - xfs_etest[i] != 0) {
> > - cleared = 1;
> > - xfs_warn(mp, "Clearing XFS error tag #%d",
> > - xfs_etest[i]);
> > - xfs_etest[i] = 0;
> > - xfs_etest_fsid[i] = 0LL;
> > - kmem_free(xfs_etest_fsname[i]);
> > - xfs_etest_fsname[i] = NULL;
> > - xfs_error_test_active--;
> > + bool cleared = false;
> > + unsigned int i;
> > +
> > + for (i = 0; i < XFS_ERRTAG_MAX; i++) {
> > + if (mp->m_errortag[i]) {
> > + cleared = true;
> > + break;
> > }
> > }
> >
> > + memset(mp->m_errortag, 0, sizeof(unsigned int) * XFS_ERRTAG_MAX);
> > +
> > if (loud || cleared)
> > xfs_warn(mp, "Cleared all XFS error tags for filesystem");
> >
> > diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> > index 05f8666..341e8a0 100644
> > --- a/fs/xfs/xfs_error.h
> > +++ b/fs/xfs/xfs_error.h
> > @@ -131,20 +131,23 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> > #define XFS_RANDOM_AG_RESV_CRITICAL 4
> >
> > #ifdef DEBUG
> > -extern int xfs_error_test_active;
> > -extern int xfs_error_test(int, int *, char *, int, char *, unsigned long);
> > -
> > -#define XFS_NUM_INJECT_ERROR 10
> > +extern int xfs_errortag_init(struct xfs_mount *mp);
> > +extern void xfs_errortag_del(struct xfs_mount *mp);
> > +extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
> > + const char *file, int line, unsigned int error_tag);
> > #define XFS_TEST_ERROR(expr, mp, tag, rf) \
> > - ((expr) || (xfs_error_test_active && \
> > - xfs_error_test((tag), (mp)->m_fixedfsid, "expr", __LINE__, __FILE__, \
> > - (rf))))
> > + ((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
> >
> > -extern int xfs_errortag_add(unsigned int error_tag, struct xfs_mount *mp);
> > -extern int xfs_errortag_clearall(struct xfs_mount *mp, int loud);
> > +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);
> > +extern int xfs_errortag_clearall(struct xfs_mount *mp, bool loud);
> > #else
> > +#define xfs_errortag_init(mp) (0)
> > +#define xfs_errortag_del(mp)
> > #define XFS_TEST_ERROR(expr, mp, tag, rf) (expr)
> > -#define xfs_errortag_add(tag, mp) (ENOSYS)
> > +#define xfs_errortag_set(mp, tag, val) (ENOSYS)
> > +#define xfs_errortag_add(mp, tag) (ENOSYS)
> > #define xfs_errortag_clearall(mp, loud) (ENOSYS)
> > #endif /* DEBUG */
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 8ffe4ea..ab0894a 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -2037,14 +2037,14 @@ xfs_file_ioctl(
> > if (copy_from_user(&in, arg, sizeof(in)))
> > return -EFAULT;
> >
> > - return xfs_errortag_add(in.errtag, mp);
> > + return xfs_errortag_add(mp, in.errtag);
> > }
> >
> > case XFS_IOC_ERROR_CLEARALL:
> > if (!capable(CAP_SYS_ADMIN))
> > return -EPERM;
> >
> > - return xfs_errortag_clearall(mp, 1);
> > + return xfs_errortag_clearall(mp, true);
> >
> > case XFS_IOC_FREE_EOFBLOCKS: {
> > struct xfs_fs_eofblocks eofb;
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index cc6789d..5cdcf43 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -720,10 +720,13 @@ xfs_mountfs(
> > if (error)
> > goto out_del_stats;
> >
> > + error = xfs_errortag_init(mp);
> > + if (error)
> > + goto out_remove_error_sysfs;
> >
> > error = xfs_uuid_mount(mp);
> > if (error)
> > - goto out_remove_error_sysfs;
> > + goto out_remove_errortag;
> >
> > /*
> > * Set the minimum read and write sizes
> > @@ -1042,6 +1045,8 @@ xfs_mountfs(
> > xfs_da_unmount(mp);
> > out_remove_uuid:
> > xfs_uuid_unmount(mp);
> > + out_remove_errortag:
> > + xfs_errortag_del(mp);
> > out_remove_error_sysfs:
> > xfs_error_sysfs_del(mp);
> > out_del_stats:
> > @@ -1145,10 +1150,11 @@ xfs_unmountfs(
> > xfs_uuid_unmount(mp);
> >
> > #if defined(DEBUG)
> > - xfs_errortag_clearall(mp, 0);
> > + xfs_errortag_clearall(mp, false);
> > #endif
> > xfs_free_perag(mp);
> >
> > + xfs_errortag_del(mp);
> > xfs_error_sysfs_del(mp);
> > xfs_sysfs_del(&mp->m_stats.xs_kobj);
> > xfs_sysfs_del(&mp->m_kobj);
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 305d953..e002ac5 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -199,6 +199,13 @@ typedef struct xfs_mount {
> > bool m_fail_unmount;
> > #ifdef DEBUG
> > /*
> > + * Frequency with which errors are injected. Replaces xfs_etest; the
> > + * value stored in here is the inverse of the frequency with which the
> > + * error triggers. 1 = always, 2 = half the time, etc.
> > + */
> > + unsigned int *m_errortag;
> > +
> > + /*
> > * DEBUG mode instrumentation to test and/or trigger delayed allocation
> > * block killing in the event of failed writes. When enabled, all
> > * buffered writes are silenty dropped and handled as if they failed.
> >
> > --
> > 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-21 18:39 ` Darrick J. Wong
@ 2017-06-21 18:53 ` Brian Foster
2017-06-21 20:45 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Brian Foster @ 2017-06-21 18:53 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Jun 21, 2017 at 11:39:55AM -0700, Darrick J. Wong wrote:
> 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.
>
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.
> 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.
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] xfs: make errortag a per-mountpoint structure
2017-06-21 18:46 ` Darrick J. Wong
@ 2017-06-21 19:05 ` Brian Foster
0 siblings, 0 replies; 20+ messages in thread
From: Brian Foster @ 2017-06-21 19:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Jun 21, 2017 at 11:46:12AM -0700, Darrick J. Wong wrote:
> On Wed, Jun 21, 2017 at 02:18:48PM -0400, Brian Foster wrote:
> > On Tue, Jun 20, 2017 at 06:11:18PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > Remove the xfs_etest structure in favor of a per-mountpoint structure.
> > > This will give us the flexibility to set as many error injection points
> > > as we want, and later enable us to set up sysfs knobs to set the trigger
> > > frequency as we wish. This comes at a cost of higher memory use, but
> > > unti we hit 1024 injection points (we're at 29) or a lot of mounts this
> > > shouldn't be a huge issue.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > fs/xfs/xfs_error.c | 161 +++++++++++++++++++++++++++++++---------------------
> > > fs/xfs/xfs_error.h | 23 ++++---
> > > fs/xfs/xfs_ioctl.c | 4 +
> > > fs/xfs/xfs_mount.c | 10 +++
> > > fs/xfs/xfs_mount.h | 7 ++
> > > 5 files changed, 125 insertions(+), 80 deletions(-)
> > >
> > >
> > > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > > index ed7ee4e..ef16ac2 100644
> > > --- a/fs/xfs/xfs_error.c
> > > +++ b/fs/xfs/xfs_error.c
> > > @@ -25,97 +25,126 @@
> > >
> > > #ifdef DEBUG
> > >
> > > -int xfs_etest[XFS_NUM_INJECT_ERROR];
> > > -int64_t xfs_etest_fsid[XFS_NUM_INJECT_ERROR];
> > > -char * xfs_etest_fsname[XFS_NUM_INJECT_ERROR];
> > > -int xfs_error_test_active;
> > > +static unsigned int xfs_errortag_random_default[] = {
> > > + XFS_RANDOM_DEFAULT,
> > > + XFS_RANDOM_IFLUSH_1,
> > > + XFS_RANDOM_IFLUSH_2,
> > > + XFS_RANDOM_IFLUSH_3,
> > > + XFS_RANDOM_IFLUSH_4,
> > > + XFS_RANDOM_IFLUSH_5,
> > > + XFS_RANDOM_IFLUSH_6,
> > > + XFS_RANDOM_DA_READ_BUF,
> > > + XFS_RANDOM_BTREE_CHECK_LBLOCK,
> > > + XFS_RANDOM_BTREE_CHECK_SBLOCK,
> > > + XFS_RANDOM_ALLOC_READ_AGF,
> > > + XFS_RANDOM_IALLOC_READ_AGI,
> > > + XFS_RANDOM_ITOBP_INOTOBP,
> > > + XFS_RANDOM_IUNLINK,
> > > + XFS_RANDOM_IUNLINK_REMOVE,
> > > + XFS_RANDOM_DIR_INO_VALIDATE,
> > > + XFS_RANDOM_BULKSTAT_READ_CHUNK,
> > > + XFS_RANDOM_IODONE_IOERR,
> > > + XFS_RANDOM_STRATREAD_IOERR,
> > > + XFS_RANDOM_STRATCMPL_IOERR,
> > > + XFS_RANDOM_DIOWRITE_IOERR,
> > > + XFS_RANDOM_BMAPIFORMAT,
> > > + XFS_RANDOM_FREE_EXTENT,
> > > + XFS_RANDOM_RMAP_FINISH_ONE,
> > > + XFS_RANDOM_REFCOUNT_CONTINUE_UPDATE,
> > > + XFS_RANDOM_REFCOUNT_FINISH_ONE,
> > > + XFS_RANDOM_BMAP_FINISH_ONE,
> > > + XFS_RANDOM_AG_RESV_CRITICAL,
> > > +};
> > >
> > > int
> > > -xfs_error_test(int error_tag, int *fsidp, char *expression,
> > > - int line, char *file, unsigned long randfactor)
> > > +xfs_errortag_init(
> > > + struct xfs_mount *mp)
> > > {
> > > - int i;
> > > - int64_t fsid;
> > > + mp->m_errortag = kmem_zalloc(sizeof(unsigned int) * XFS_ERRTAG_MAX,
> > > + KM_SLEEP | KM_MAYFAIL);
> > > + if (!mp->m_errortag)
> > > + return -ENOMEM;
> > > + return 0;
> > > +}
> > >
> > > - if (prandom_u32() % randfactor)
> > > - return 0;
> > > +void
> > > +xfs_errortag_del(
> > > + struct xfs_mount *mp)
> > > +{
> > > + kmem_free(mp->m_errortag);
> > > +}
> > >
> > > - memcpy(&fsid, fsidp, sizeof(xfs_fsid_t));
> > > +bool
> > > +xfs_errortag_test(
> > > + struct xfs_mount *mp,
> > > + const char *expression,
> > > + const char *file,
> > > + int line,
> > > + unsigned int error_tag)
> > > +{
> > > + unsigned int randfactor;
> > >
> > > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > > - if (xfs_etest[i] == error_tag && xfs_etest_fsid[i] == fsid) {
> > > - xfs_warn(NULL,
> > > - "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> > > - expression, file, line, xfs_etest_fsname[i]);
> > > - return 1;
> > > - }
> > > - }
> > > + ASSERT(error_tag < XFS_ERRTAG_MAX);
> > > + randfactor = mp->m_errortag[error_tag];
> > > + if (!randfactor || prandom_u32() % randfactor)
> > > + return false;
> > >
> > > - return 0;
> > > + xfs_warn(mp,
> > > +"Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> > > + expression, file, line, mp->m_fsname);
> > > + return true;
> >
> > I wonder if we're going to want/need to ratelimit this now that we have
> > dynamic frequency support. I'm thinking cases like 100% error frequency
> > for the purpose of AIL item pinning could generate quite a bit of
> > output.
>
> Yeah, we should xfs_warn_ratelimited, the one I made to force repair
> to repair undamaged metadata is particularly noisy.
>
> >
> > > }
> > >
> > > int
> > > -xfs_errortag_add(unsigned int error_tag, xfs_mount_t *mp)
> > > +xfs_errortag_set(
> > > + struct xfs_mount *mp,
> > > + unsigned int error_tag,
> > > + unsigned int tag_value)
> > > {
> > > - int i;
> > > - int len;
> > > - int64_t fsid;
> > > -
> > > if (error_tag >= XFS_ERRTAG_MAX)
> > > return -EINVAL;
> > >
> > > - memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
> > > + mp->m_errortag[error_tag] = tag_value;
> > > + xfs_warn(mp,
> > > +"Turned %s XFS error tag #%d",
> > > + tag_value ? "on" : "off", error_tag);
> > > + return 0;
> >
> > This is unnecessarily noisy IMO as well. If we wanted to preserve it for
> > the ioctl() case, we could pull it up into the caller. Unless it's
> > actually used somewhere that I'm not aware of, I'd be fine with killing
> > it altogether.
>
> Sure. We cough out an error message every time we actually inject
> an error anyway.
>
> >
> > > +}
> > >
> > > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > > - if (xfs_etest_fsid[i] == fsid && xfs_etest[i] == error_tag) {
> > > - xfs_warn(mp, "error tag #%d on", error_tag);
> > > - return 0;
> > > - }
> > > - }
> > > +int
> > > +xfs_errortag_add(
> > > + struct xfs_mount *mp,
> > > + unsigned int error_tag)
> > > +{
> > > + if (error_tag >= XFS_ERRTAG_MAX)
> > > + return -EINVAL;
> > >
> > > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > > - if (xfs_etest[i] == 0) {
> > > - xfs_warn(mp, "Turned on XFS error tag #%d",
> > > - error_tag);
> > > - xfs_etest[i] = error_tag;
> > > - xfs_etest_fsid[i] = fsid;
> > > - len = strlen(mp->m_fsname);
> > > - xfs_etest_fsname[i] = kmem_alloc(len + 1, KM_SLEEP);
> > > - strcpy(xfs_etest_fsname[i], mp->m_fsname);
> > > - xfs_error_test_active++;
> > > - return 0;
> > > - }
> > > + if (mp->m_errortag[error_tag]) {
> > > + xfs_warn(mp, "error tag #%d on", error_tag);
> > > + return 0;
> >
> > It probably doesn't matter that much, but I wonder if we should still
> > set the _random_default value even if the tag is already enabled. The
>
> The ioctl errored out if the tag was already enabled, so this preserves
> that behavior. Note that xfs_errortag_add is only called by the ioctl,
> whereas xfs_errortag_set is called by sysfs and it doesn't care what
> the previous value was.
>
Yeah, that's what I figured. I'm more weighing whether it's better for
the ioctl() to force set the value rather than error out. Previously,
returning an error implied what the value was (whatever the hardcoded
value is). With the new interface being available, it just tells us that
it's on and the value could be anything.
> Granted, maybe we don't care about checking if it's already set and
> should just go ahead and change the value, but that would change the
> behavior of an existing (debug) interface.
>
It would be a slight change, indeed. Granted this is all debug code so
I'm less hesitant about preserving things like error semantics and
whatnot. I think it's probably best to just go with whatever seems more
useful/sane. I don't care too much either way for that same reason, I
just wanted to call it out for discussion.
Brian
> > idea being to (sort of) preserve backwards compatibility for the ioctl()
> > in the event that the tag was already enabled via sysfs.
> >
> > Now that I think of it, does the ioctl() interface even support turning
> > off an individual error tag? :/
>
> Correct. You only can clear all of them, at least via ioctls.
>
> --D
>
> >
> > Brian
> >
> > > }
> > >
> > > - xfs_warn(mp, "error tag overflow, too many turned on");
> > > -
> > > - return 1;
> > > + return xfs_errortag_set(mp, error_tag,
> > > + xfs_errortag_random_default[error_tag]);
> > > }
> > >
> > > int
> > > -xfs_errortag_clearall(xfs_mount_t *mp, int loud)
> > > +xfs_errortag_clearall(
> > > + struct xfs_mount *mp,
> > > + bool loud)
> > > {
> > > - int64_t fsid;
> > > - int cleared = 0;
> > > - int i;
> > > -
> > > - memcpy(&fsid, mp->m_fixedfsid, sizeof(xfs_fsid_t));
> > > -
> > > -
> > > - for (i = 0; i < XFS_NUM_INJECT_ERROR; i++) {
> > > - if ((fsid == 0LL || xfs_etest_fsid[i] == fsid) &&
> > > - xfs_etest[i] != 0) {
> > > - cleared = 1;
> > > - xfs_warn(mp, "Clearing XFS error tag #%d",
> > > - xfs_etest[i]);
> > > - xfs_etest[i] = 0;
> > > - xfs_etest_fsid[i] = 0LL;
> > > - kmem_free(xfs_etest_fsname[i]);
> > > - xfs_etest_fsname[i] = NULL;
> > > - xfs_error_test_active--;
> > > + bool cleared = false;
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < XFS_ERRTAG_MAX; i++) {
> > > + if (mp->m_errortag[i]) {
> > > + cleared = true;
> > > + break;
> > > }
> > > }
> > >
> > > + memset(mp->m_errortag, 0, sizeof(unsigned int) * XFS_ERRTAG_MAX);
> > > +
> > > if (loud || cleared)
> > > xfs_warn(mp, "Cleared all XFS error tags for filesystem");
> > >
> > > diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> > > index 05f8666..341e8a0 100644
> > > --- a/fs/xfs/xfs_error.h
> > > +++ b/fs/xfs/xfs_error.h
> > > @@ -131,20 +131,23 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> > > #define XFS_RANDOM_AG_RESV_CRITICAL 4
> > >
> > > #ifdef DEBUG
> > > -extern int xfs_error_test_active;
> > > -extern int xfs_error_test(int, int *, char *, int, char *, unsigned long);
> > > -
> > > -#define XFS_NUM_INJECT_ERROR 10
> > > +extern int xfs_errortag_init(struct xfs_mount *mp);
> > > +extern void xfs_errortag_del(struct xfs_mount *mp);
> > > +extern bool xfs_errortag_test(struct xfs_mount *mp, const char *expression,
> > > + const char *file, int line, unsigned int error_tag);
> > > #define XFS_TEST_ERROR(expr, mp, tag, rf) \
> > > - ((expr) || (xfs_error_test_active && \
> > > - xfs_error_test((tag), (mp)->m_fixedfsid, "expr", __LINE__, __FILE__, \
> > > - (rf))))
> > > + ((expr) || xfs_errortag_test((mp), #expr, __FILE__, __LINE__, (tag)))
> > >
> > > -extern int xfs_errortag_add(unsigned int error_tag, struct xfs_mount *mp);
> > > -extern int xfs_errortag_clearall(struct xfs_mount *mp, int loud);
> > > +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);
> > > +extern int xfs_errortag_clearall(struct xfs_mount *mp, bool loud);
> > > #else
> > > +#define xfs_errortag_init(mp) (0)
> > > +#define xfs_errortag_del(mp)
> > > #define XFS_TEST_ERROR(expr, mp, tag, rf) (expr)
> > > -#define xfs_errortag_add(tag, mp) (ENOSYS)
> > > +#define xfs_errortag_set(mp, tag, val) (ENOSYS)
> > > +#define xfs_errortag_add(mp, tag) (ENOSYS)
> > > #define xfs_errortag_clearall(mp, loud) (ENOSYS)
> > > #endif /* DEBUG */
> > >
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index 8ffe4ea..ab0894a 100644
> > > --- a/fs/xfs/xfs_ioctl.c
> > > +++ b/fs/xfs/xfs_ioctl.c
> > > @@ -2037,14 +2037,14 @@ xfs_file_ioctl(
> > > if (copy_from_user(&in, arg, sizeof(in)))
> > > return -EFAULT;
> > >
> > > - return xfs_errortag_add(in.errtag, mp);
> > > + return xfs_errortag_add(mp, in.errtag);
> > > }
> > >
> > > case XFS_IOC_ERROR_CLEARALL:
> > > if (!capable(CAP_SYS_ADMIN))
> > > return -EPERM;
> > >
> > > - return xfs_errortag_clearall(mp, 1);
> > > + return xfs_errortag_clearall(mp, true);
> > >
> > > case XFS_IOC_FREE_EOFBLOCKS: {
> > > struct xfs_fs_eofblocks eofb;
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index cc6789d..5cdcf43 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
> > > @@ -720,10 +720,13 @@ xfs_mountfs(
> > > if (error)
> > > goto out_del_stats;
> > >
> > > + error = xfs_errortag_init(mp);
> > > + if (error)
> > > + goto out_remove_error_sysfs;
> > >
> > > error = xfs_uuid_mount(mp);
> > > if (error)
> > > - goto out_remove_error_sysfs;
> > > + goto out_remove_errortag;
> > >
> > > /*
> > > * Set the minimum read and write sizes
> > > @@ -1042,6 +1045,8 @@ xfs_mountfs(
> > > xfs_da_unmount(mp);
> > > out_remove_uuid:
> > > xfs_uuid_unmount(mp);
> > > + out_remove_errortag:
> > > + xfs_errortag_del(mp);
> > > out_remove_error_sysfs:
> > > xfs_error_sysfs_del(mp);
> > > out_del_stats:
> > > @@ -1145,10 +1150,11 @@ xfs_unmountfs(
> > > xfs_uuid_unmount(mp);
> > >
> > > #if defined(DEBUG)
> > > - xfs_errortag_clearall(mp, 0);
> > > + xfs_errortag_clearall(mp, false);
> > > #endif
> > > xfs_free_perag(mp);
> > >
> > > + xfs_errortag_del(mp);
> > > xfs_error_sysfs_del(mp);
> > > xfs_sysfs_del(&mp->m_stats.xs_kobj);
> > > xfs_sysfs_del(&mp->m_kobj);
> > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > index 305d953..e002ac5 100644
> > > --- a/fs/xfs/xfs_mount.h
> > > +++ b/fs/xfs/xfs_mount.h
> > > @@ -199,6 +199,13 @@ typedef struct xfs_mount {
> > > bool m_fail_unmount;
> > > #ifdef DEBUG
> > > /*
> > > + * Frequency with which errors are injected. Replaces xfs_etest; the
> > > + * value stored in here is the inverse of the frequency with which the
> > > + * error triggers. 1 = always, 2 = half the time, etc.
> > > + */
> > > + unsigned int *m_errortag;
> > > +
> > > + /*
> > > * DEBUG mode instrumentation to test and/or trigger delayed allocation
> > > * block killing in the event of failed writes. When enabled, all
> > > * buffered writes are silenty dropped and handled as if they failed.
> > >
> > > --
> > > 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-21 18:53 ` Brian Foster
@ 2017-06-21 20:45 ` Darrick J. Wong
2017-06-22 15:15 ` Carlos Maiolino
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-21 20:45 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Wed, Jun 21, 2017 at 02:53:54PM -0400, Brian Foster wrote:
> On Wed, Jun 21, 2017 at 11:39:55AM -0700, Darrick J. Wong wrote:
> > 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.
> >
>
> 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".
> > 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-21 20:45 ` Darrick J. Wong
@ 2017-06-22 15:15 ` Carlos Maiolino
2017-06-22 17:29 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Carlos Maiolino @ 2017-06-22 15:15 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Brian Foster, linux-xfs
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?
> > > > > +
> > > > > +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 :)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-22 15:15 ` Carlos Maiolino
@ 2017-06-22 17:29 ` Darrick J. Wong
2017-06-23 9:16 ` Carlos Maiolino
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-22 17:29 UTC (permalink / raw)
To: Brian Foster, linux-xfs
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-22 17:29 ` Darrick J. Wong
@ 2017-06-23 9:16 ` Carlos Maiolino
2017-06-23 16:13 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Carlos Maiolino @ 2017-06-23 9:16 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Brian Foster, linux-xfs
> > 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
>
Fair enough.
> > > > > > > +
> > > > > > > +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. :)
>
I agree with you here, I can give a hand deprecating the old ioctl in the
hopefully not too far future if you want, just give me a heads up.
Regarding the whole patch series with these error knobs, I liked it, it I
discussed it a bit with Brian yesterday, and as we already spoke about, will be
useful to even implement a xfstests regarding the dm-thin stuff I'm working on.
I have the xfstests for that case almost done though, using fsfreeze to test the
bug fix.
Do you plan to remove the RFC flag from this patchset soon? I can wait in this
case and send a xfstests based on this feature (I'll need to implement some
extra knob to force a buffer error, but that is simple with this patchset), or
just keep the current plan with fsfreeze (suggested by Brian btw), send it ASAP,
and update it in the future.
What you think?
Cheers
> (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
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: expose errortag knobs via sysfs
2017-06-23 9:16 ` Carlos Maiolino
@ 2017-06-23 16:13 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-23 16:13 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On Fri, Jun 23, 2017 at 11:16:06AM +0200, Carlos Maiolino wrote:
> > > 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
> >
> Fair enough.
>
> > > > > > > > +
> > > > > > > > +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. :)
> >
> I agree with you here, I can give a hand deprecating the old ioctl in the
> hopefully not too far future if you want, just give me a heads up.
>
> Regarding the whole patch series with these error knobs, I liked it, it I
> discussed it a bit with Brian yesterday, and as we already spoke about, will be
> useful to even implement a xfstests regarding the dm-thin stuff I'm working on.
>
> I have the xfstests for that case almost done though, using fsfreeze to test the
> bug fix.
>
> Do you plan to remove the RFC flag from this patchset soon? I can wait in this
> case and send a xfstests based on this feature (I'll need to implement some
> extra knob to force a buffer error, but that is simple with this patchset), or
> just keep the current plan with fsfreeze (suggested by Brian btw), send it ASAP,
> and update it in the future.
I'll send a v2 without the RFC tag later today. If I get r-v-b's for
the first two patches then I'm open to putting the rework into 4.13
since it's a debugging feature and (afaik) we're still at least a week
away from the merge window. I've been running xfstests on for-next +
the RFC patches and haven't seen any regressions in the tests that use
the injector.
--D
>
> What you think?
>
> Cheers
>
> > (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
> > --
> > 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism
2017-06-23 16:35 ` [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism Darrick J. Wong
@ 2017-06-27 10:51 ` Carlos Maiolino
0 siblings, 0 replies; 20+ messages in thread
From: Carlos Maiolino @ 2017-06-27 10:51 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Brian Foster
On Fri, Jun 23, 2017 at 09:35:42AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> We now have enhanced error injection that can control the frequency
> with which errors happen, so convert drop_writes to use this.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/xfs_error.c | 3 +++
> fs/xfs/xfs_error.h | 14 +++++++++++++-
> fs/xfs/xfs_iomap.c | 2 +-
> fs/xfs/xfs_mount.h | 24 ------------------------
> fs/xfs/xfs_sysfs.c | 42 ------------------------------------------
> 5 files changed, 17 insertions(+), 68 deletions(-)
>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index e2278af6..a2f23d2 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -55,6 +55,7 @@ static unsigned int xfs_errortag_random_default[] = {
> XFS_RANDOM_REFCOUNT_FINISH_ONE,
> XFS_RANDOM_BMAP_FINISH_ONE,
> XFS_RANDOM_AG_RESV_CRITICAL,
> + XFS_RANDOM_DROP_WRITES,
> };
>
> struct xfs_errortag_attr {
> @@ -157,6 +158,7 @@ XFS_ERRORTAG_ATTR_RW(refcount_continue_update, XFS_ERRTAG_REFCOUNT_CONTINUE_UPDA
> 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);
> +XFS_ERRORTAG_ATTR_RW(drop_writes, XFS_ERRTAG_DROP_WRITES);
>
> static struct attribute *xfs_errortag_attrs[] = {
> XFS_ERRORTAG_ATTR_LIST(noerror),
> @@ -187,6 +189,7 @@ static struct attribute *xfs_errortag_attrs[] = {
> XFS_ERRORTAG_ATTR_LIST(refcount_finish_one),
> XFS_ERRORTAG_ATTR_LIST(bmap_finish_one),
> XFS_ERRORTAG_ATTR_LIST(ag_resv_critical),
> + XFS_ERRORTAG_ATTR_LIST(drop_writes),
> NULL,
> };
>
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index ae8935b..ad9b24a 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -96,7 +96,18 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> #define XFS_ERRTAG_REFCOUNT_FINISH_ONE 25
> #define XFS_ERRTAG_BMAP_FINISH_ONE 26
> #define XFS_ERRTAG_AG_RESV_CRITICAL 27
> -#define XFS_ERRTAG_MAX 28
> +
> +/*
> + * DEBUG mode instrumentation to test and/or trigger delayed allocation
> + * block killing in the event of failed writes. When enabled, all
> + * buffered writes are silenty dropped and handled as if they failed.
> + * All delalloc blocks in the range of the write (including pre-existing
> + * delalloc blocks!) are tossed as part of the write failure error
> + * handling sequence.
> + */
> +#define XFS_ERRTAG_DROP_WRITES 28
> +
> +#define XFS_ERRTAG_MAX 29
>
> /*
> * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
> @@ -129,6 +140,7 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
> #define XFS_RANDOM_REFCOUNT_FINISH_ONE 1
> #define XFS_RANDOM_BMAP_FINISH_ONE 1
> #define XFS_RANDOM_AG_RESV_CRITICAL 4
> +#define XFS_RANDOM_DROP_WRITES 1
>
> #ifdef DEBUG
> extern int xfs_errortag_init(struct xfs_mount *mp);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 304b79d..86f1a9f 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1097,7 +1097,7 @@ xfs_file_iomap_end_delalloc(
> * Behave as if the write failed if drop writes is enabled. Set the NEW
> * flag to force delalloc cleanup.
> */
> - if (xfs_mp_drop_writes(mp)) {
> + if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
> iomap->flags |= IOMAP_F_NEW;
> written = 0;
> }
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 931e9fc..e0792d0 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -205,16 +205,6 @@ typedef struct xfs_mount {
> */
> unsigned int *m_errortag;
> struct xfs_kobj m_errortag_kobj;
> -
> - /*
> - * DEBUG mode instrumentation to test and/or trigger delayed allocation
> - * block killing in the event of failed writes. When enabled, all
> - * buffered writes are silenty dropped and handled as if they failed.
> - * All delalloc blocks in the range of the write (including pre-existing
> - * delalloc blocks!) are tossed as part of the write failure error
> - * handling sequence.
> - */
> - bool m_drop_writes;
> #endif
> } xfs_mount_t;
>
> @@ -333,20 +323,6 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d)
> return (xfs_agblock_t) do_div(ld, mp->m_sb.sb_agblocks);
> }
>
> -#ifdef DEBUG
> -static inline bool
> -xfs_mp_drop_writes(struct xfs_mount *mp)
> -{
> - return mp->m_drop_writes;
> -}
> -#else
> -static inline bool
> -xfs_mp_drop_writes(struct xfs_mount *mp)
> -{
> - return 0;
> -}
> -#endif
> -
> /* per-AG block reservation data structures*/
> enum xfs_ag_resv_type {
> XFS_AG_RESV_NONE = 0,
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index ec6e0e2..56610a9 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -90,49 +90,7 @@ to_mp(struct kobject *kobject)
> return container_of(kobj, struct xfs_mount, m_kobj);
> }
>
> -#ifdef DEBUG
> -
> -STATIC ssize_t
> -drop_writes_store(
> - struct kobject *kobject,
> - const char *buf,
> - size_t count)
> -{
> - struct xfs_mount *mp = to_mp(kobject);
> - int ret;
> - int val;
> -
> - ret = kstrtoint(buf, 0, &val);
> - if (ret)
> - return ret;
> -
> - if (val == 1)
> - mp->m_drop_writes = true;
> - else if (val == 0)
> - mp->m_drop_writes = false;
> - else
> - return -EINVAL;
> -
> - return count;
> -}
> -
> -STATIC ssize_t
> -drop_writes_show(
> - struct kobject *kobject,
> - char *buf)
> -{
> - struct xfs_mount *mp = to_mp(kobject);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n", mp->m_drop_writes ? 1 : 0);
> -}
> -XFS_SYSFS_ATTR_RW(drop_writes);
> -
> -#endif /* DEBUG */
> -
> static struct attribute *xfs_mp_attrs[] = {
> -#ifdef DEBUG
> - ATTR_LIST(drop_writes),
> -#endif
> NULL,
> };
>
>
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism
2017-06-23 16:35 [PATCH v2 0/4] xfs: more configurable error injection Darrick J. Wong
@ 2017-06-23 16:35 ` Darrick J. Wong
2017-06-27 10:51 ` Carlos Maiolino
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2017-06-23 16:35 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, Brian Foster
From: Darrick J. Wong <darrick.wong@oracle.com>
We now have enhanced error injection that can control the frequency
with which errors happen, so convert drop_writes to use this.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_error.c | 3 +++
fs/xfs/xfs_error.h | 14 +++++++++++++-
fs/xfs/xfs_iomap.c | 2 +-
fs/xfs/xfs_mount.h | 24 ------------------------
fs/xfs/xfs_sysfs.c | 42 ------------------------------------------
5 files changed, 17 insertions(+), 68 deletions(-)
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index e2278af6..a2f23d2 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -55,6 +55,7 @@ static unsigned int xfs_errortag_random_default[] = {
XFS_RANDOM_REFCOUNT_FINISH_ONE,
XFS_RANDOM_BMAP_FINISH_ONE,
XFS_RANDOM_AG_RESV_CRITICAL,
+ XFS_RANDOM_DROP_WRITES,
};
struct xfs_errortag_attr {
@@ -157,6 +158,7 @@ XFS_ERRORTAG_ATTR_RW(refcount_continue_update, XFS_ERRTAG_REFCOUNT_CONTINUE_UPDA
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);
+XFS_ERRORTAG_ATTR_RW(drop_writes, XFS_ERRTAG_DROP_WRITES);
static struct attribute *xfs_errortag_attrs[] = {
XFS_ERRORTAG_ATTR_LIST(noerror),
@@ -187,6 +189,7 @@ static struct attribute *xfs_errortag_attrs[] = {
XFS_ERRORTAG_ATTR_LIST(refcount_finish_one),
XFS_ERRORTAG_ATTR_LIST(bmap_finish_one),
XFS_ERRORTAG_ATTR_LIST(ag_resv_critical),
+ XFS_ERRORTAG_ATTR_LIST(drop_writes),
NULL,
};
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index ae8935b..ad9b24a 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -96,7 +96,18 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
#define XFS_ERRTAG_REFCOUNT_FINISH_ONE 25
#define XFS_ERRTAG_BMAP_FINISH_ONE 26
#define XFS_ERRTAG_AG_RESV_CRITICAL 27
-#define XFS_ERRTAG_MAX 28
+
+/*
+ * DEBUG mode instrumentation to test and/or trigger delayed allocation
+ * block killing in the event of failed writes. When enabled, all
+ * buffered writes are silenty dropped and handled as if they failed.
+ * All delalloc blocks in the range of the write (including pre-existing
+ * delalloc blocks!) are tossed as part of the write failure error
+ * handling sequence.
+ */
+#define XFS_ERRTAG_DROP_WRITES 28
+
+#define XFS_ERRTAG_MAX 29
/*
* Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -129,6 +140,7 @@ extern void xfs_verifier_error(struct xfs_buf *bp);
#define XFS_RANDOM_REFCOUNT_FINISH_ONE 1
#define XFS_RANDOM_BMAP_FINISH_ONE 1
#define XFS_RANDOM_AG_RESV_CRITICAL 4
+#define XFS_RANDOM_DROP_WRITES 1
#ifdef DEBUG
extern int xfs_errortag_init(struct xfs_mount *mp);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 304b79d..86f1a9f 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1097,7 +1097,7 @@ xfs_file_iomap_end_delalloc(
* Behave as if the write failed if drop writes is enabled. Set the NEW
* flag to force delalloc cleanup.
*/
- if (xfs_mp_drop_writes(mp)) {
+ if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
iomap->flags |= IOMAP_F_NEW;
written = 0;
}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 931e9fc..e0792d0 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -205,16 +205,6 @@ typedef struct xfs_mount {
*/
unsigned int *m_errortag;
struct xfs_kobj m_errortag_kobj;
-
- /*
- * DEBUG mode instrumentation to test and/or trigger delayed allocation
- * block killing in the event of failed writes. When enabled, all
- * buffered writes are silenty dropped and handled as if they failed.
- * All delalloc blocks in the range of the write (including pre-existing
- * delalloc blocks!) are tossed as part of the write failure error
- * handling sequence.
- */
- bool m_drop_writes;
#endif
} xfs_mount_t;
@@ -333,20 +323,6 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d)
return (xfs_agblock_t) do_div(ld, mp->m_sb.sb_agblocks);
}
-#ifdef DEBUG
-static inline bool
-xfs_mp_drop_writes(struct xfs_mount *mp)
-{
- return mp->m_drop_writes;
-}
-#else
-static inline bool
-xfs_mp_drop_writes(struct xfs_mount *mp)
-{
- return 0;
-}
-#endif
-
/* per-AG block reservation data structures*/
enum xfs_ag_resv_type {
XFS_AG_RESV_NONE = 0,
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index ec6e0e2..56610a9 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -90,49 +90,7 @@ to_mp(struct kobject *kobject)
return container_of(kobj, struct xfs_mount, m_kobj);
}
-#ifdef DEBUG
-
-STATIC ssize_t
-drop_writes_store(
- struct kobject *kobject,
- const char *buf,
- size_t count)
-{
- struct xfs_mount *mp = to_mp(kobject);
- int ret;
- int val;
-
- ret = kstrtoint(buf, 0, &val);
- if (ret)
- return ret;
-
- if (val == 1)
- mp->m_drop_writes = true;
- else if (val == 0)
- mp->m_drop_writes = false;
- else
- return -EINVAL;
-
- return count;
-}
-
-STATIC ssize_t
-drop_writes_show(
- struct kobject *kobject,
- char *buf)
-{
- struct xfs_mount *mp = to_mp(kobject);
-
- return snprintf(buf, PAGE_SIZE, "%d\n", mp->m_drop_writes ? 1 : 0);
-}
-XFS_SYSFS_ATTR_RW(drop_writes);
-
-#endif /* DEBUG */
-
static struct attribute *xfs_mp_attrs[] = {
-#ifdef DEBUG
- ATTR_LIST(drop_writes),
-#endif
NULL,
};
^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-06-27 10:51 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-21 1:11 [RFC PATCH 0/4] xfs: more configurable error injection Darrick J. Wong
2017-06-21 1:11 ` [PATCH 1/4] xfs: make errortag a per-mountpoint structure Darrick J. Wong
2017-06-21 18:18 ` Brian Foster
2017-06-21 18:46 ` Darrick J. Wong
2017-06-21 19:05 ` Brian Foster
2017-06-21 1:11 ` [PATCH 2/4] xfs: expose errortag knobs via sysfs Darrick J. Wong
2017-06-21 18:19 ` Brian Foster
2017-06-21 18:39 ` Darrick J. Wong
2017-06-21 18:53 ` Brian Foster
2017-06-21 20:45 ` Darrick J. Wong
2017-06-22 15:15 ` Carlos Maiolino
2017-06-22 17:29 ` Darrick J. Wong
2017-06-23 9:16 ` Carlos Maiolino
2017-06-23 16:13 ` Darrick J. Wong
2017-06-21 1:11 ` [PATCH 3/4] xfs: remove unneeded parameter from XFS_TEST_ERROR Darrick J. Wong
2017-06-21 18:19 ` Brian Foster
2017-06-21 1:11 ` [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism Darrick J. Wong
2017-06-21 18:19 ` Brian Foster
2017-06-23 16:35 [PATCH v2 0/4] xfs: more configurable error injection Darrick J. Wong
2017-06-23 16:35 ` [PATCH 4/4] xfs: convert drop_writes to use the errortag mechanism Darrick J. Wong
2017-06-27 10:51 ` Carlos Maiolino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.