| Message ID | 1566282838-2980-1-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v2] syscalls/statx04: use stx_attributes_mask before test | expand |
Hi Yang, > stx_attributes_mask shows what's supported in stx_attributes. > we should check four attrbutes whether supports on tested filesystem typo attrbutes > and only test supported flags on tested filesystem. I'd change it to Set supp_{append,compr,immutable,nodump} attributes only on filesystems which actually support them. > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> > --- > testcases/kernel/syscalls/statx/statx04.c | 124 ++++++++++++++++------ ... > - attr |= FS_COMPR_FL | FS_APPEND_FL | FS_IMMUTABLE_FL | FS_NODUMP_FL; > + if (supp_compr) > + attr |= FS_COMPR_FL; > + if (supp_append) > + attr |= FS_APPEND_FL; > + if (supp_immutable) > + attr |= FS_IMMUTABLE_FL; > + if (supp_nodump) > + attr |= FS_NODUMP_FL; > ret = ioctl(fd, FS_IOC_SETFLAGS, &attr); > if (ret < 0) { > @@ -149,12 +176,43 @@ static void caid_flags_setup(void) Current code... if (supp_append) attr |= FS_APPEND_FL; if (supp_compr) attr |= FS_COMPR_FL; if (supp_immutable) attr |= FS_IMMUTABLE_FL; if (supp_nodump) attr |= FS_NODUMP_FL; ret = ioctl(fd, FS_IOC_SETFLAGS, &attr); if (ret < 0) { I wonder, if this check is still needed. Probably it's still useful to have sanity check, but "Flags not supported" has been caught by supp_{append,compr,immutable,nodump} variables. if (errno == EOPNOTSUPP) tst_brk(TCONF, "Flags not supported"); tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_SETFLAGS, %i)", fd, attr); } ... Kind regards, Petr
Hi Yang, also Cyril noted on Github issue #557 [1]: "However I'm not sure that we should do this since kernel devs declared this to be bug after all." So maybe we should wait before applying it. @Cyril: can you please post link to discussion in LKML? Kind regards, Petr [1] https://github.com/linux-test-project/ltp/issues/557#issuecomment-522593891
Hi! > also Cyril noted on Github issue #557 [1]: > "However I'm not sure that we should do this since kernel devs declared this to be bug after all." > > So maybe we should wait before applying it. > > @Cyril: can you please post link to discussion in LKML? I've talked to Jan Kara in person while developing these tests, so there is no track of this discussion. I guess that the best we can do here is to: * Apply this patch, since the test should generally check only for flags that are supported in the bitflag returned by the syscall * Add another test that checks that these bitflags are enabled for new enough kernels for certain set of filesystems
on 2019/08/27 17:25, Petr Vorel wrote: > Hi Yang, > >> stx_attributes_mask shows what's supported in stx_attributes. >> we should check four attrbutes whether supports on tested filesystem > typo attrbutes >> and only test supported flags on tested filesystem. > I'd change it to > Set supp_{append,compr,immutable,nodump} attributes only on filesystems which > actually support them. > >> Signed-off-by: Yang Xu<xuyang2018.jy@cn.fujitsu.com> > Reviewed-by: Petr Vorel<pvorel@suse.cz> Hi Petr Thanks for your review. >> --- >> testcases/kernel/syscalls/statx/statx04.c | 124 ++++++++++++++++------ > ... > >> - attr |= FS_COMPR_FL | FS_APPEND_FL | FS_IMMUTABLE_FL | FS_NODUMP_FL; >> + if (supp_compr) >> + attr |= FS_COMPR_FL; >> + if (supp_append) >> + attr |= FS_APPEND_FL; >> + if (supp_immutable) >> + attr |= FS_IMMUTABLE_FL; >> + if (supp_nodump) >> + attr |= FS_NODUMP_FL; >> ret = ioctl(fd, FS_IOC_SETFLAGS,&attr); >> if (ret< 0) { >> @@ -149,12 +176,43 @@ static void caid_flags_setup(void) > Current code... > if (supp_append) > attr |= FS_APPEND_FL; > if (supp_compr) > attr |= FS_COMPR_FL; > if (supp_immutable) > attr |= FS_IMMUTABLE_FL; > if (supp_nodump) > attr |= FS_NODUMP_FL; > > ret = ioctl(fd, FS_IOC_SETFLAGS,&attr); > if (ret< 0) { > I wonder, if this check is still needed. Probably it's still useful to have > sanity check, but "Flags not supported" has been caught > by supp_{append,compr,immutable,nodump} variables. It seems this check is redundant. In principle, if attributes_mask support these flags, the attribute should also support them. Even though xfs filesystem missed attributes_mask after[1] and doesn't add it until [2]. But we don't have the situation of having attribute_mask but not having attribute. So I think we can remove it. [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5f955f26f3d42d [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1b9598c8fb9965 Thanks Yang Xu > if (errno == EOPNOTSUPP) > tst_brk(TCONF, "Flags not supported"); > tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_SETFLAGS, %i)", fd, attr); > } > ... > > Kind regards, > Petr > > > . >
on 2019/08/27 18:16, Cyril Hrubis wrote: > Hi! >> also Cyril noted on Github issue #557 [1]: >> "However I'm not sure that we should do this since kernel devs declared this to be bug after all." >> >> So maybe we should wait before applying it. >> >> @Cyril: can you please post link to discussion in LKML? > I've talked to Jan Kara in person while developing these tests, so there > is no track of this discussion. > > I guess that the best we can do here is to: > > * Apply this patch, since the test should generally check only for flags > that are supported in the bitflag returned by the syscall > > * Add another test that checks that these bitflags are enabled for new > enough kernels for certain set of filesystems Hi Cyril Do you mean use getxattr to ensure bitflags are enable or a functions test? I am confused. Thanks Yang Xu >
Hi! > Do you mean use getxattr to ensure bitflags are enable or a functions test? > I am confused. For a given filesystem the support for filling in these flags was added at some point to the kernel. If any kernel newer that this version fails to fill them up it's a bug. For ext2 it has been added in: commit 93bc420ed41df63a18ae794101f7cbf45226a6ef Author: yangerkun <yangerkun@huawei.com> Date: Mon Feb 18 09:07:02 2019 +0800 ext2: support statx syscall Hence starting kernel 5.0 ext2 (with ext2 driver) has to set the mask. For ext4 it has been added in: commit 3209f68b3ca4667069923a325c88b21131bfdf9f Author: David Howells <dhowells@redhat.com> Date: Fri Mar 31 18:32:17 2017 +0100 statx: Include a mask for stx_attributes in struct statx Hence for ext4 the flags should be enabled since kernel 4.11 etc.
on 2019/09/11 20:47, Cyril Hrubis wrote: > Hi! >> Do you mean use getxattr to ensure bitflags are enable or a functions test? >> I am confused. > For a given filesystem the support for filling in these flags was added > at some point to the kernel. If any kernel newer that this version fails > to fill them up it's a bug. > > For ext2 it has been added in: > > commit 93bc420ed41df63a18ae794101f7cbf45226a6ef > Author: yangerkun <yangerkun@huawei.com> > Date: Mon Feb 18 09:07:02 2019 +0800 > > ext2: support statx syscall > > Hence starting kernel 5.0 ext2 (with ext2 driver) has to set the mask. > > For ext4 it has been added in: > > commit 3209f68b3ca4667069923a325c88b21131bfdf9f > Author: David Howells <dhowells@redhat.com> > Date: Fri Mar 31 18:32:17 2017 +0100 > > statx: Include a mask for stx_attributes in struct statx > > > Hence for ext4 the flags should be enabled since kernel 4.11 Hi Cyril Thanks, I see. It seems that kernel version check is useful for upstream kernel. But if an LTS linux distribution backports the commit 93bc420 for ext2, this kernel version will make no sense. I remember ltp has a discussion between EISDIR and EBDAF about copy_file_range[1]. Also ext2 should enable CONFIG_EXT2_FS_XATTR if we don't use ext4 instead of it. IMO, we don't need to add kernel enable version check for various filesystems because QA or user can find their system doesn't support these flags and report this to linux distribution vendor. So these flags may be supported on next release. If kernel enables these flags fails to fill them up it's a bug. We can only give some comments about their enabled commit information. So user can know it is a bug or a non-supported feature. ps: I have a question about min_kernel all the time. If a new feature(such as PR_CAP_AMBIENT ) is introduced since upstream kernel 4.3, but this feature is also backported on low version kernel on some linux distributions. What kind of situation can we use the min_kernel to distinguish it? what kind of situation we don't need? test-writing-guidelines.txt doesn't mention it. Thanks Yang Xu > > etc. >
diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c index 71de734f5..7b462c3f9 100644 --- a/testcases/kernel/syscalls/statx/statx04.c +++ b/testcases/kernel/syscalls/statx/statx04.c @@ -34,6 +34,10 @@ #define TESTDIR_UNFLAGGED MOUNT_POINT"/test_dir2" static int fd, clear_flags; +static int supp_compr; +static int supp_append; +static int supp_immutable; +static int supp_nodump; static void test_flagged(void) { @@ -47,25 +51,33 @@ static void test_flagged(void) tst_brk(TFAIL | TTERRNO, "sys_statx(AT_FDCWD, %s, 0, 0, &buf)", TESTDIR_FLAGGED); - if (buf.stx_attributes & STATX_ATTR_COMPRESSED) - tst_res(TPASS, "STATX_ATTR_COMPRESSED flag is set"); - else - tst_res(TFAIL, "STATX_ATTR_COMPRESSED flag is not set"); + if (supp_compr) { + if (buf.stx_attributes & STATX_ATTR_COMPRESSED) + tst_res(TPASS, "STATX_ATTR_COMPRESSED flag is set"); + else + tst_res(TFAIL, "STATX_ATTR_COMPRESSED flag is not set"); + } - if (buf.stx_attributes & STATX_ATTR_APPEND) - tst_res(TPASS, "STATX_ATTR_APPEND flag is set"); - else - tst_res(TFAIL, "STATX_ATTR_APPEND flag is not set"); + if (supp_append) { + if (buf.stx_attributes & STATX_ATTR_APPEND) + tst_res(TPASS, "STATX_ATTR_APPEND flag is set"); + else + tst_res(TFAIL, "STATX_ATTR_APPEND flag is not set"); + } - if (buf.stx_attributes & STATX_ATTR_IMMUTABLE) - tst_res(TPASS, "STATX_ATTR_IMMUTABLE flag is set"); - else - tst_res(TFAIL, "STATX_ATTR_IMMUTABLE flag is not set"); + if (supp_immutable) { + if (buf.stx_attributes & STATX_ATTR_IMMUTABLE) + tst_res(TPASS, "STATX_ATTR_IMMUTABLE flag is set"); + else + tst_res(TFAIL, "STATX_ATTR_IMMUTABLE flag is not set"); + } - if (buf.stx_attributes & STATX_ATTR_NODUMP) - tst_res(TPASS, "STATX_ATTR_NODUMP flag is set"); - else - tst_res(TFAIL, "STATX_ATTR_NODUMP flag is not set"); + if (supp_nodump) { + if (buf.stx_attributes & STATX_ATTR_NODUMP) + tst_res(TPASS, "STATX_ATTR_NODUMP flag is set"); + else + tst_res(TFAIL, "STATX_ATTR_NODUMP flag is not set"); + } } static void test_unflagged(void) @@ -82,25 +94,33 @@ static void test_unflagged(void) "sys_statx(AT_FDCWD, %s, 0, 0, &buf)", TESTDIR_UNFLAGGED); - if ((buf.stx_attributes & STATX_ATTR_COMPRESSED) == 0) - tst_res(TPASS, "STATX_ATTR_COMPRESSED flag is not set"); - else - tst_res(TFAIL, "STATX_ATTR_COMPRESSED flag is set"); + if (supp_compr) { + if ((buf.stx_attributes & STATX_ATTR_COMPRESSED) == 0) + tst_res(TPASS, "STATX_ATTR_COMPRESSED flag is not set"); + else + tst_res(TFAIL, "STATX_ATTR_COMPRESSED flag is set"); + } - if ((buf.stx_attributes & STATX_ATTR_APPEND) == 0) - tst_res(TPASS, "STATX_ATTR_APPEND flag is not set"); - else - tst_res(TFAIL, "STATX_ATTR_APPEND flag is set"); + if (supp_append) { + if ((buf.stx_attributes & STATX_ATTR_APPEND) == 0) + tst_res(TPASS, "STATX_ATTR_APPEND flag is not set"); + else + tst_res(TFAIL, "STATX_ATTR_APPEND flag is set"); + } - if ((buf.stx_attributes & STATX_ATTR_IMMUTABLE) == 0) - tst_res(TPASS, "STATX_ATTR_IMMUTABLE flag is not set"); - else - tst_res(TFAIL, "STATX_ATTR_IMMUTABLE flag is set"); + if (supp_immutable) { + if ((buf.stx_attributes & STATX_ATTR_IMMUTABLE) == 0) + tst_res(TPASS, "STATX_ATTR_IMMUTABLE flag is not set"); + else + tst_res(TFAIL, "STATX_ATTR_IMMUTABLE flag is set"); + } - if ((buf.stx_attributes & STATX_ATTR_NODUMP) == 0) - tst_res(TPASS, "STATX_ATTR_NODUMP flag is not set"); - else - tst_res(TFAIL, "STATX_ATTR_NODUMP flag is set"); + if (supp_nodump) { + if ((buf.stx_attributes & STATX_ATTR_NODUMP) == 0) + tst_res(TPASS, "STATX_ATTR_NODUMP flag is not set"); + else + tst_res(TFAIL, "STATX_ATTR_NODUMP flag is set"); + } } struct test_cases { @@ -135,7 +155,14 @@ static void caid_flags_setup(void) tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_GETFLAGS, ...)", fd); } - attr |= FS_COMPR_FL | FS_APPEND_FL | FS_IMMUTABLE_FL | FS_NODUMP_FL; + if (supp_compr) + attr |= FS_COMPR_FL; + if (supp_append) + attr |= FS_APPEND_FL; + if (supp_immutable) + attr |= FS_IMMUTABLE_FL; + if (supp_nodump) + attr |= FS_NODUMP_FL; ret = ioctl(fd, FS_IOC_SETFLAGS, &attr); if (ret < 0) { @@ -149,12 +176,43 @@ static void caid_flags_setup(void) static void setup(void) { + struct statx buf; + + supp_compr = 1; + supp_append = 1; + supp_immutable = 1; + supp_nodump = 1; SAFE_MKDIR(TESTDIR_FLAGGED, 0777); SAFE_MKDIR(TESTDIR_UNFLAGGED, 0777); if (!strcmp(tst_device->fs_type, "btrfs") && tst_kvercmp(4, 13, 0) < 0) tst_brk(TCONF, "Btrfs statx() supported since 4.13"); + TEST(statx(AT_FDCWD, TESTDIR_FLAGGED, 0, 0, &buf)); + if (TST_RET == -1) + tst_brk(TFAIL | TTERRNO, + "sys_statx(AT_FDCWD, %s, 0, 0, &buf)", TESTDIR_FLAGGED); + + if ((buf.stx_attributes_mask & FS_COMPR_FL) == 0) { + supp_compr = 0; + tst_res(TCONF, "filesystem doesn't support FS_COMPR_FL"); + } + if ((buf.stx_attributes_mask & FS_APPEND_FL) == 0) { + supp_append = 0; + tst_res(TCONF, "filesystem doesn't support FS_APPEND_FL"); + } + if ((buf.stx_attributes_mask & FS_IMMUTABLE_FL) == 0) { + supp_immutable = 0; + tst_res(TCONF, "filesystem doesn't support FS_IMMUTABLE_FL"); + } + if ((buf.stx_attributes_mask & FS_NODUMP_FL) == 0) { + supp_nodump = 0; + tst_res(TCONF, "filesystem doesn't support FS_NODUMP_FL"); + } + if (!(supp_compr || supp_append || supp_immutable || supp_nodump)) + tst_brk(TCONF, + "filesystem doesn't support the above any attr, skip it"); + caid_flags_setup(); }
stx_attributes_mask shows what's supported in stx_attributes. we should check four attrbutes whether supports on tested filesystem and only test supported flags on tested filesystem. Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- testcases/kernel/syscalls/statx/statx04.c | 124 ++++++++++++++++------ 1 file changed, 91 insertions(+), 33 deletions(-)