| Message ID | 20230825122753.4721-1-jack@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fanotify22: Make tests not depend on behavior of shutdown filesystem | expand |
On Fri, Aug 25, 2023 at 3:28 PM Jan Kara <jack@suse.cz> wrote: > > The tests in fanotify22 implicitely depended on the fact that filesystem > shutdown with 'abort' mount option keeps reporting further errors and > further mounts with 'abort' option. This is however too strict (mostly a > bug in ext4 implementation) and in principle reporting errors after the > filesystem is shutdown is just a pointless noise. Ext4 recently modified > the behavior of 'abort' mount option to behave the same as filesystem > shutdown and thus also stop reporting further filesystem errors. Modify > the tests to unmount and mount the filesystem after each test to get it > out of the shutdown state for the following tests and also replace a > test testing behavior after mounting with 'abort' mount option with a > test testing two different filesystem corruption errors. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > .../kernel/syscalls/fanotify/fanotify22.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify22.c b/testcases/kernel/syscalls/fanotify/fanotify22.c > index 1105172bb269..475155b9f58a 100644 > --- a/testcases/kernel/syscalls/fanotify/fanotify22.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c > @@ -42,6 +42,7 @@ > #define MOUNT_PATH "test_mnt" > #define BASE_DIR "internal_dir" > #define BAD_DIR BASE_DIR"/bad_dir" > +#define BAD_LINK BASE_DIR"/bad_link" > > #ifdef HAVE_NAME_TO_HANDLE_AT > > @@ -51,6 +52,7 @@ static int fd_notify; > /* These expected FIDs are common to multiple tests */ > static struct fanotify_fid_t null_fid; > static struct fanotify_fid_t bad_file_fid; > +static struct fanotify_fid_t bad_link_fid; > > static void trigger_fs_abort(void) > { > @@ -78,7 +80,13 @@ static void tcase2_trigger_lookup(void) > > static void tcase3_trigger(void) > { > - trigger_fs_abort(); > + int ret; > + > + /* SAFE_OPEN cannot be used here because we expect it to fail. */ > + ret = open(MOUNT_PATH"/"BAD_LINK, O_RDONLY, 0); > + if (ret != -1 && errno != EUCLEAN) > + tst_res(TFAIL, "Unexpected open result(%d) of %s (%d!=%d)", > + ret, BAD_LINK, errno, EUCLEAN); > tcase2_trigger_lookup(); > } To make it more clear that this is a multiple error trigger, I would consider 1. use helper trigger_bad_link_lookup() 2. s/tcase2_trigger_lookup/trigger_bad_file_lookup AND static void tcase3_trigger(void) { trigger_bad_link_lookup(); trigger_bad_file_lookup(); } With that nit fix, you may add: Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > @@ -113,8 +121,8 @@ static struct test_case { > .name = "Multiple error submission", > .trigger_error = &tcase3_trigger, > .error_count = 2, > - .error = ESHUTDOWN, > - .fid = &null_fid, > + .error = EFSCORRUPTED, > + .fid = &bad_link_fid, > }, > { > .name = "Multiple error submission 2", > @@ -248,6 +256,9 @@ static void do_test(unsigned int i) > FAN_FS_ERROR, AT_FDCWD, MOUNT_PATH); > > check_event(event_buf, read_len, tcase); > + /* Unmount and mount the filesystem to get it out of the error state */ > + SAFE_UMOUNT(MOUNT_PATH); > + SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL); > } > > static void pre_corrupt_fs(void) > @@ -256,9 +267,11 @@ static void pre_corrupt_fs(void) > SAFE_MKDIR(MOUNT_PATH"/"BAD_DIR, 0777); > > fanotify_save_fid(MOUNT_PATH"/"BAD_DIR, &bad_file_fid); > + fanotify_save_fid(MOUNT_PATH"/"BASE_DIR, &bad_link_fid); > > SAFE_UMOUNT(MOUNT_PATH); > do_debugfs_request(tst_device->dev, "sif " BAD_DIR " mode 0xff"); > + do_debugfs_request(tst_device->dev, "ln <1> " BAD_LINK); > SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL); > } > > -- > 2.35.3 >
On Fri 25-08-23 17:12:17, Amir Goldstein wrote: > On Fri, Aug 25, 2023 at 3:28 PM Jan Kara <jack@suse.cz> wrote: > > > > The tests in fanotify22 implicitely depended on the fact that filesystem > > shutdown with 'abort' mount option keeps reporting further errors and > > further mounts with 'abort' option. This is however too strict (mostly a > > bug in ext4 implementation) and in principle reporting errors after the > > filesystem is shutdown is just a pointless noise. Ext4 recently modified > > the behavior of 'abort' mount option to behave the same as filesystem > > shutdown and thus also stop reporting further filesystem errors. Modify > > the tests to unmount and mount the filesystem after each test to get it > > out of the shutdown state for the following tests and also replace a > > test testing behavior after mounting with 'abort' mount option with a > > test testing two different filesystem corruption errors. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > .../kernel/syscalls/fanotify/fanotify22.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify22.c b/testcases/kernel/syscalls/fanotify/fanotify22.c > > index 1105172bb269..475155b9f58a 100644 > > --- a/testcases/kernel/syscalls/fanotify/fanotify22.c > > +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c > > @@ -42,6 +42,7 @@ > > #define MOUNT_PATH "test_mnt" > > #define BASE_DIR "internal_dir" > > #define BAD_DIR BASE_DIR"/bad_dir" > > +#define BAD_LINK BASE_DIR"/bad_link" > > > > #ifdef HAVE_NAME_TO_HANDLE_AT > > > > @@ -51,6 +52,7 @@ static int fd_notify; > > /* These expected FIDs are common to multiple tests */ > > static struct fanotify_fid_t null_fid; > > static struct fanotify_fid_t bad_file_fid; > > +static struct fanotify_fid_t bad_link_fid; > > > > static void trigger_fs_abort(void) > > { > > @@ -78,7 +80,13 @@ static void tcase2_trigger_lookup(void) > > > > static void tcase3_trigger(void) > > { > > - trigger_fs_abort(); > > + int ret; > > + > > + /* SAFE_OPEN cannot be used here because we expect it to fail. */ > > + ret = open(MOUNT_PATH"/"BAD_LINK, O_RDONLY, 0); > > + if (ret != -1 && errno != EUCLEAN) > > + tst_res(TFAIL, "Unexpected open result(%d) of %s (%d!=%d)", > > + ret, BAD_LINK, errno, EUCLEAN); > > tcase2_trigger_lookup(); > > } > > To make it more clear that this is a multiple error trigger, I would consider > > 1. use helper trigger_bad_link_lookup() > 2. s/tcase2_trigger_lookup/trigger_bad_file_lookup > > AND > > static void tcase3_trigger(void) > { > trigger_bad_link_lookup(); > trigger_bad_file_lookup(); > } > > With that nit fix, you may add: > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> Thanks for review! I did all the changes, submitting v2 shortly. Honza
diff --git a/testcases/kernel/syscalls/fanotify/fanotify22.c b/testcases/kernel/syscalls/fanotify/fanotify22.c index 1105172bb269..475155b9f58a 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify22.c +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c @@ -42,6 +42,7 @@ #define MOUNT_PATH "test_mnt" #define BASE_DIR "internal_dir" #define BAD_DIR BASE_DIR"/bad_dir" +#define BAD_LINK BASE_DIR"/bad_link" #ifdef HAVE_NAME_TO_HANDLE_AT @@ -51,6 +52,7 @@ static int fd_notify; /* These expected FIDs are common to multiple tests */ static struct fanotify_fid_t null_fid; static struct fanotify_fid_t bad_file_fid; +static struct fanotify_fid_t bad_link_fid; static void trigger_fs_abort(void) { @@ -78,7 +80,13 @@ static void tcase2_trigger_lookup(void) static void tcase3_trigger(void) { - trigger_fs_abort(); + int ret; + + /* SAFE_OPEN cannot be used here because we expect it to fail. */ + ret = open(MOUNT_PATH"/"BAD_LINK, O_RDONLY, 0); + if (ret != -1 && errno != EUCLEAN) + tst_res(TFAIL, "Unexpected open result(%d) of %s (%d!=%d)", + ret, BAD_LINK, errno, EUCLEAN); tcase2_trigger_lookup(); } @@ -113,8 +121,8 @@ static struct test_case { .name = "Multiple error submission", .trigger_error = &tcase3_trigger, .error_count = 2, - .error = ESHUTDOWN, - .fid = &null_fid, + .error = EFSCORRUPTED, + .fid = &bad_link_fid, }, { .name = "Multiple error submission 2", @@ -248,6 +256,9 @@ static void do_test(unsigned int i) FAN_FS_ERROR, AT_FDCWD, MOUNT_PATH); check_event(event_buf, read_len, tcase); + /* Unmount and mount the filesystem to get it out of the error state */ + SAFE_UMOUNT(MOUNT_PATH); + SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL); } static void pre_corrupt_fs(void) @@ -256,9 +267,11 @@ static void pre_corrupt_fs(void) SAFE_MKDIR(MOUNT_PATH"/"BAD_DIR, 0777); fanotify_save_fid(MOUNT_PATH"/"BAD_DIR, &bad_file_fid); + fanotify_save_fid(MOUNT_PATH"/"BASE_DIR, &bad_link_fid); SAFE_UMOUNT(MOUNT_PATH); do_debugfs_request(tst_device->dev, "sif " BAD_DIR " mode 0xff"); + do_debugfs_request(tst_device->dev, "ln <1> " BAD_LINK); SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL); }
The tests in fanotify22 implicitely depended on the fact that filesystem shutdown with 'abort' mount option keeps reporting further errors and further mounts with 'abort' option. This is however too strict (mostly a bug in ext4 implementation) and in principle reporting errors after the filesystem is shutdown is just a pointless noise. Ext4 recently modified the behavior of 'abort' mount option to behave the same as filesystem shutdown and thus also stop reporting further filesystem errors. Modify the tests to unmount and mount the filesystem after each test to get it out of the shutdown state for the following tests and also replace a test testing behavior after mounting with 'abort' mount option with a test testing two different filesystem corruption errors. Signed-off-by: Jan Kara <jack@suse.cz> --- .../kernel/syscalls/fanotify/fanotify22.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)