diff mbox series

[v2,3/3] statx04: Skip STATX_ATTR_COMPRESSED on Bcachefs

Message ID 20231207194011.273027-4-pvorel@suse.cz (mailing list archive)
State New, archived
Headers show
Series Add support bcachefs filesystem | expand

Commit Message

Petr Vorel Dec. 7, 2023, 7:40 p.m. UTC
STATX_ATTR_COMPRESSED is not supported on Bcachefs, thus skip it
(it's already skipped on tmpfs and XFS).

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New in v2

 testcases/kernel/syscalls/statx/statx04.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Kent Overstreet Dec. 13, 2023, 2:46 a.m. UTC | #1
On Thu, Dec 07, 2023 at 08:40:11PM +0100, Petr Vorel wrote:
> STATX_ATTR_COMPRESSED is not supported on Bcachefs, thus skip it
> (it's already skipped on tmpfs and XFS).

Hang on, bcachefs most definitely does hae compression. This would be
just because statx isn't plumbed through?

> 
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> New in v2
> 
>  testcases/kernel/syscalls/statx/statx04.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c
> index 58296bd24..c2ed52deb 100644
> --- a/testcases/kernel/syscalls/statx/statx04.c
> +++ b/testcases/kernel/syscalls/statx/statx04.c
> @@ -96,8 +96,9 @@ static void setup(void)
>  	for (i = 0, expected_mask = 0; i < ARRAY_SIZE(attr_list); i++)
>  		expected_mask |= attr_list[i].attr;
>  
> -	/* STATX_ATTR_COMPRESSED not supported on XFS, TMPFS */
> -	if (!strcmp(tst_device->fs_type, "xfs") || !strcmp(tst_device->fs_type, "tmpfs"))
> +	/* STATX_ATTR_COMPRESSED not supported on Bcachefs, TMPFS, XFS */
> +	if (!strcmp(tst_device->fs_type, "bcachefs") || !strcmp(tst_device->fs_type, "tmpfs") ||
> +	    !strcmp(tst_device->fs_type, "xfs"))
>  		expected_mask &= ~STATX_ATTR_COMPRESSED;
>  
>  	/* Attribute support was added to Btrfs statx() in kernel v4.13 */
> -- 
> 2.43.0
>
Cyril Hrubis Jan. 8, 2024, 10:01 a.m. UTC | #2
Hi!
> On Thu, Dec 07, 2023 at 08:40:11PM +0100, Petr Vorel wrote:
> > STATX_ATTR_COMPRESSED is not supported on Bcachefs, thus skip it
> > (it's already skipped on tmpfs and XFS).
> 
> Hang on, bcachefs most definitely does hae compression. This would be
> just because statx isn't plumbed through?

Quite likely, other filesystems does have an inode flag that is set when
file has been compressed and simply report that in the foo_getattr()
callback. Looking at bcachefs I supose that we need to figure out if the
inode is v3 and then unpack the v3 info to get to the compressed flag,
you probably know best how to do that.
Kent Overstreet Jan. 8, 2024, 8:54 p.m. UTC | #3
On Mon, Jan 08, 2024 at 11:01:40AM +0100, Cyril Hrubis wrote:
> Hi!
> > On Thu, Dec 07, 2023 at 08:40:11PM +0100, Petr Vorel wrote:
> > > STATX_ATTR_COMPRESSED is not supported on Bcachefs, thus skip it
> > > (it's already skipped on tmpfs and XFS).
> > 
> > Hang on, bcachefs most definitely does hae compression. This would be
> > just because statx isn't plumbed through?
> 
> Quite likely, other filesystems does have an inode flag that is set when
> file has been compressed and simply report that in the foo_getattr()
> callback. Looking at bcachefs I supose that we need to figure out if the
> inode is v3 and then unpack the v3 info to get to the compressed flag,
> you probably know best how to do that.

I'm still not clear how we want to map STATX_ATTR_COMPRESSED, since it's
extents that are compressed, not entire files - and just reporting the
compression option is probably not what we want since it can be flipped
off, and existing data will still be compressed.

Do you know anything about the intended use case?
Cyril Hrubis Jan. 9, 2024, 10:32 a.m. UTC | #4
Hi!
> > Quite likely, other filesystems does have an inode flag that is set when
> > file has been compressed and simply report that in the foo_getattr()
> > callback. Looking at bcachefs I supose that we need to figure out if the
> > inode is v3 and then unpack the v3 info to get to the compressed flag,
> > you probably know best how to do that.
> 
> I'm still not clear how we want to map STATX_ATTR_COMPRESSED, since it's
> extents that are compressed, not entire files - and just reporting the
> compression option is probably not what we want since it can be flipped
> off, and existing data will still be compressed.
> 
> Do you know anything about the intended use case?

As far as I understand the flag it's a hint that the file I/O may be
slower/need more memory because of the compression.
Cyril Hrubis April 19, 2024, 1:11 p.m. UTC | #5
Hi!
> > > Quite likely, other filesystems does have an inode flag that is set when
> > > file has been compressed and simply report that in the foo_getattr()
> > > callback. Looking at bcachefs I supose that we need to figure out if the
> > > inode is v3 and then unpack the v3 info to get to the compressed flag,
> > > you probably know best how to do that.
> > 
> > I'm still not clear how we want to map STATX_ATTR_COMPRESSED, since it's
> > extents that are compressed, not entire files - and just reporting the
> > compression option is probably not what we want since it can be flipped
> > off, and existing data will still be compressed.
> > 
> > Do you know anything about the intended use case?
> 
> As far as I understand the flag it's a hint that the file I/O may be
> slower/need more memory because of the compression.

Ping.

Kent we are getting closer to a next LTP release, are you going to add
the compressed flag support into bcachefs or should we add bcachefs to
the test skiplist?
Petr Vorel May 7, 2024, 8:18 a.m. UTC | #6
> Hi!
> > > > Quite likely, other filesystems does have an inode flag that is set when
> > > > file has been compressed and simply report that in the foo_getattr()
> > > > callback. Looking at bcachefs I supose that we need to figure out if the
> > > > inode is v3 and then unpack the v3 info to get to the compressed flag,
> > > > you probably know best how to do that.

> > > I'm still not clear how we want to map STATX_ATTR_COMPRESSED, since it's
> > > extents that are compressed, not entire files - and just reporting the
> > > compression option is probably not what we want since it can be flipped
> > > off, and existing data will still be compressed.

> > > Do you know anything about the intended use case?

> > As far as I understand the flag it's a hint that the file I/O may be
> > slower/need more memory because of the compression.

> Ping.

> Kent we are getting closer to a next LTP release, are you going to add
> the compressed flag support into bcachefs or should we add bcachefs to
> the test skiplist?

Kent, gentle ping ^.

Kind regards,
Petr
Kent Overstreet May 7, 2024, 2:16 p.m. UTC | #7
On Tue, May 07, 2024 at 10:18:46AM +0200, Petr Vorel wrote:
> > Hi!
> > > > > Quite likely, other filesystems does have an inode flag that is set when
> > > > > file has been compressed and simply report that in the foo_getattr()
> > > > > callback. Looking at bcachefs I supose that we need to figure out if the
> > > > > inode is v3 and then unpack the v3 info to get to the compressed flag,
> > > > > you probably know best how to do that.
> 
> > > > I'm still not clear how we want to map STATX_ATTR_COMPRESSED, since it's
> > > > extents that are compressed, not entire files - and just reporting the
> > > > compression option is probably not what we want since it can be flipped
> > > > off, and existing data will still be compressed.
> 
> > > > Do you know anything about the intended use case?
> 
> > > As far as I understand the flag it's a hint that the file I/O may be
> > > slower/need more memory because of the compression.
> 
> > Ping.
> 
> > Kent we are getting closer to a next LTP release, are you going to add
> > the compressed flag support into bcachefs or should we add bcachefs to
> > the test skiplist?
> 
> Kent, gentle ping ^.

Add it to the skiplist for now, thanks
Petr Vorel May 7, 2024, 5:03 p.m. UTC | #8
> On Tue, May 07, 2024 at 10:18:46AM +0200, Petr Vorel wrote:
> > > Hi!
> > > > > > Quite likely, other filesystems does have an inode flag that is set when
> > > > > > file has been compressed and simply report that in the foo_getattr()
> > > > > > callback. Looking at bcachefs I supose that we need to figure out if the
> > > > > > inode is v3 and then unpack the v3 info to get to the compressed flag,
> > > > > > you probably know best how to do that.

> > > > > I'm still not clear how we want to map STATX_ATTR_COMPRESSED, since it's
> > > > > extents that are compressed, not entire files - and just reporting the
> > > > > compression option is probably not what we want since it can be flipped
> > > > > off, and existing data will still be compressed.

> > > > > Do you know anything about the intended use case?

> > > > As far as I understand the flag it's a hint that the file I/O may be
> > > > slower/need more memory because of the compression.

> > > Ping.

> > > Kent we are getting closer to a next LTP release, are you going to add
> > > the compressed flag support into bcachefs or should we add bcachefs to
> > > the test skiplist?

> > Kent, gentle ping ^.

> Add it to the skiplist for now, thanks

Thanks, whitelist merged with your ABT.

Kind regards,
Petr
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c
index 58296bd24..c2ed52deb 100644
--- a/testcases/kernel/syscalls/statx/statx04.c
+++ b/testcases/kernel/syscalls/statx/statx04.c
@@ -96,8 +96,9 @@  static void setup(void)
 	for (i = 0, expected_mask = 0; i < ARRAY_SIZE(attr_list); i++)
 		expected_mask |= attr_list[i].attr;
 
-	/* STATX_ATTR_COMPRESSED not supported on XFS, TMPFS */
-	if (!strcmp(tst_device->fs_type, "xfs") || !strcmp(tst_device->fs_type, "tmpfs"))
+	/* STATX_ATTR_COMPRESSED not supported on Bcachefs, TMPFS, XFS */
+	if (!strcmp(tst_device->fs_type, "bcachefs") || !strcmp(tst_device->fs_type, "tmpfs") ||
+	    !strcmp(tst_device->fs_type, "xfs"))
 		expected_mask &= ~STATX_ATTR_COMPRESSED;
 
 	/* Attribute support was added to Btrfs statx() in kernel v4.13 */