[f2fs-dev] [DISCUSSION] f2fs: compatibility between compressed files and large folios

[f2fs-dev] [DISCUSSION] f2fs: compatibility between compressed files and large folios

[Nanzhe Zhao via Linux-f2fs-devel]

Hi all,

Chao Yu and I discussed the compatibility between compressed files and large
folios.

The main race scenario is that f2fs_new_inode() creates an inode without the
compression flag and enables large-folio support for its mapping. Later,
f2fs_setflags_common() can set the compression flag while large-folio support
is already enabled for the inode. Neither f2fs_iget() nor f2fs_new_inode()
can prevent this for an active inode.

Here are the two approaches I am considering.

1. Reject setting the compression flag on a large-folio mapping

We can reject setting the compression flag in f2fs_setflags_common() if the
inode mapping supports large folios:

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2250,6 +2250,9 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
        if ((iflags ^ masked_flags) & F2FS_COMPR_FL) {
                if (masked_flags & F2FS_COMPR_FL) {
                        if (!f2fs_disable_compressed_file(inode))
                                return -EINVAL;
                } else {
+                       if (mapping_large_folio_support(inode->i_mapping))
+                               return -EOPNOTSUPP;
+
                        /* try to convert inline_data to support compression */
                        int err = f2fs_convert_inline_inode(inode);
                        if (err)
                                return err;

Another implementation is:

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2250,6 +2250,9 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
        if ((iflags ^ masked_flags) & F2FS_COMPR_FL) {
                if (masked_flags & F2FS_COMPR_FL) {
                        if (!f2fs_disable_compressed_file(inode))
                                return -EINVAL;
                } else {
+                       if (IS_ENABLED(CONFIG_F2FS_LARGE_FOLIO))
+                               return -EOPNOTSUPP;
+
                        /* try to convert inline_data to support compression */
                        int err = f2fs_convert_inline_inode(inode);
                        if (err)
                                return err;

The concern with this approach is that new files trying to set compression
through the ioctl will be rejected, so compression will not be available. I
think we can document that enabling large-folio support in f2fs disables
compression.

2. Keep the compression flag and restore order 0

If the compression flag and compression functionality need to remain
available, we can restore the mapping order to 0 after set_compress_context()
successfully sets the compression flag in f2fs_setflags_common():

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2268,6 +2268,10 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
                                return -EINVAL;
                        }
                        err = set_compress_context(inode);
+                       if (!err &&
+                           mapping_large_folio_support(inode->i_mapping))
+                               mapping_set_folio_order_range(
+                                       inode->i_mapping, 0, 0);
                        f2fs_up_write(&fi->i_sem);

                        if (err)
                                return err;

Under normal Android workloads, a file whose compression flag can be set by
f2fs_setflags_common() has no data blocks. Also, an application will not read
a newly created file with size 0 and fill the page cache with zeroed large
folios. Therefore, setting the mapping order to 0 here is safe.

A theoretical sequence is: create a file, use ftruncate() to extend it to a
non-zero size without allocating data blocks, read the file holes so that
zeroed large folios are left in the page cache, and then set the compression
flag through the ioctl. If truncate_inode_pages() is not called before the
mapping order is reset, the existing large folios remain in the page cache.
Also, since mapping_set_folio_order_range() is not atomic, readahead may
theoretically access the mapping's folio-order flags without holding the inode
lock. Neither case is expected under normal Android workloads.

Jaegeuk previously mentioned that a file could have both the compression flag
and a large-folio mapping. Its writeback could use the normal large-folio path
without actually compressing the file. After the inode is evicted and the file
is opened again, f2fs_iget() would see the compression flag, prevent the
large-folio mapping from being enabled, and return to the normal order-0 read
path. Chao Yu pointed out that it would be functionally strange if a file
with the compression flag was not compressed during its first writeback.

Please let us know which of the above approaches you think is better, or
whether we have another better approach.

Thanks,

Nanzhe



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [DISCUSSION] f2fs: compatibility between compressed files and large folios

[Chao Yu via Linux-f2fs-devel]

Nanzhe,

Thanks for the proposal.

On 7/29/26 08:28, Nanzhe Zhao wrote:
> Hi all,
> 
> Chao Yu and I discussed the compatibility between compressed files and large
> folios.
> 
> The main race scenario is that f2fs_new_inode() creates an inode without the
> compression flag and enables large-folio support for its mapping. Later,
> f2fs_setflags_common() can set the compression flag while large-folio support
> is already enabled for the inode. Neither f2fs_iget() nor f2fs_new_inode()
> can prevent this for an active inode.
> 
> Here are the two approaches I am considering.
> 
> 1. Reject setting the compression flag on a large-folio mapping
> 
> We can reject setting the compression flag in f2fs_setflags_common() if the
> inode mapping supports large folios:
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2250,6 +2250,9 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>          if ((iflags ^ masked_flags) & F2FS_COMPR_FL) {
>                  if (masked_flags & F2FS_COMPR_FL) {
>                          if (!f2fs_disable_compressed_file(inode))
>                                  return -EINVAL;
>                  } else {
> +                       if (mapping_large_folio_support(inode->i_mapping))
> +                               return -EOPNOTSUPP;
> +
>                          /* try to convert inline_data to support compression */
>                          int err = f2fs_convert_inline_inode(inode);
>                          if (err)
>                                  return err;
> 
> Another implementation is:
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2250,6 +2250,9 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>          if ((iflags ^ masked_flags) & F2FS_COMPR_FL) {
>                  if (masked_flags & F2FS_COMPR_FL) {
>                          if (!f2fs_disable_compressed_file(inode))
>                                  return -EINVAL;
>                  } else {
> +                       if (IS_ENABLED(CONFIG_F2FS_LARGE_FOLIO))
> +                               return -EOPNOTSUPP;
> +
>                          /* try to convert inline_data to support compression */
>                          int err = f2fs_convert_inline_inode(inode);
>                          if (err)
>                                  return err;
> 
> The concern with this approach is that new files trying to set compression
> through the ioctl will be rejected, so compression will not be available. I
> think we can document that enabling large-folio support in f2fs disables
> compression.
> 
> 2. Keep the compression flag and restore order 0
> 
> If the compression flag and compression functionality need to remain
> available, we can restore the mapping order to 0 after set_compress_context()
> successfully sets the compression flag in f2fs_setflags_common():
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2268,6 +2268,10 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>                                  return -EINVAL;
>                          }
>                          err = set_compress_context(inode);
> +                       if (!err &&
> +                           mapping_large_folio_support(inode->i_mapping))
> +                               mapping_set_folio_order_range(
> +                                       inode->i_mapping, 0, 0);

I prefer this solution, it keeps f2fs compression functionality available, rather
than just disabling it after we enable large folio. I suspect there is existing
users: applications are using it, I don't want to break any userspace use.

>                          f2fs_up_write(&fi->i_sem);
> 
>                          if (err)
>                                  return err;
> 
> Under normal Android workloads, a file whose compression flag can be set by
> f2fs_setflags_common() has no data blocks. Also, an application will not read
> a newly created file with size 0 and fill the page cache with zeroed large
> folios. Therefore, setting the mapping order to 0 here is safe.
> 
> A theoretical sequence is: create a file, use ftruncate() to extend it to a
> non-zero size without allocating data blocks, read the file holes so that
> zeroed large folios are left in the page cache, and then set the compression
> flag through the ioctl. If truncate_inode_pages() is not called before the
> mapping order is reset, the existing large folios remain in the page cache.
> Also, since mapping_set_folio_order_range() is not atomic, readahead may
> theoretically access the mapping's folio-order flags without holding the inode
> lock. Neither case is expected under normal Android workloads.

Can we call truncate_inode_pages() after mapping_set_folio_order_range()? something
like this:

		filemap_invalidate_lock

		mapping_set_folio_order_range(, 0, 0)
		truncate_inode_pages()

		if (mapping->nrpages) {
			err = -EBUSY;
			goto out_unlock;
		}

		...
		err = set_compress_context(inode);
		...

		filemap_invalidate_unlock

Thanks,

> 
> Jaegeuk previously mentioned that a file could have both the compression flag
> and a large-folio mapping. Its writeback could use the normal large-folio path
> without actually compressing the file. After the inode is evicted and the file
> is opened again, f2fs_iget() would see the compression flag, prevent the
> large-folio mapping from being enabled, and return to the normal order-0 read
> path. Chao Yu pointed out that it would be functionally strange if a file
> with the compression flag was not compressed during its first writeback.
> 
> Please let us know which of the above approaches you think is better, or
> whether we have another better approach.
> 
> Thanks,
> 
> Nanzhe
> 



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [DISCUSSION] f2fs: compatibility between compressed files and large folios

[Nanzhe Zhao via Linux-f2fs-devel]

Chao,

Thanks for the suggestion.

I am just a bit concerned about whether using it this way would violate
its intended design contract. Its comment states:

* Context: This should not be called while the inode is active as it
* is non-atomic.

Our current use case is exactly the scenario where the inode is in an
active state :)

Jaegeuk, could you also share your thoughts on this?

Thanks,

Nanzhe



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [DISCUSSION] f2fs: compatibility between compressed files and large folios

[Nanzhe Zhao via Linux-f2fs-devel]

The quote part of last email to reply is:

> Can we call truncate_inode_pages() after mapping_set_folio_order_range()? something
> like this:
>
>                 filemap_invalidate_lock
>
>                 mapping_set_folio_order_range(, 0, 0)

Thanks,

Nanzhe



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [DISCUSSION] f2fs: compatibility between compressed files and large folios

[Jaegeuk Kim via Linux-f2fs-devel]

On 07/31, Nanzhe Zhao via Linux-f2fs-devel wrote:
> The quote part of last email to reply is:
> 
> > Can we call truncate_inode_pages() after mapping_set_folio_order_range()? something
> > like this:
> >
> >                 filemap_invalidate_lock
> >
> >                 mapping_set_folio_order_range(, 0, 0)
> 
> Thanks,
> 
> Nanzhe

From the previous experience, this won't work, as you mentioned it's not allowed
to change the order on the fly. Should set the order in f2fs_iget() only, but
it'd be better to keep supporting compression. :)

> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel