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

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel