Re: [Devel] [PATCH vz10 v3] fs/ext4: let FALLOC_FL_PREALLOCATE through the vfs_fallocate mode switch

[Konstantin Khorenko <khorenko@virtuozzo.com>]

1. Append-only files, pre-existing.

A few lines below the switch the patch touched, vfs_fallocate() still has:

      /*
       * On append-only files only space preallocation is supported.
       */
      if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode))
              return -EPERM;

so fallocate(0) works on an append-only file while
fallocate(FALLOC_FL_PREALLOCATE) gets EPERM, even though the comment
says space preallocation is the thing that is allowed and that is
exactly what the new mode does.

vz9 behaved the same way, so this is not a regression, but probably need
to fix it?

2. One flag combination stopped working.

      fallocate(fd, FALLOC_FL_ZERO_RANGE | FALLOC_FL_PREALLOCATE, ...)

worked in vz9 and returns EOPNOTSUPP now. That mode value is 0x90, and
since both bits sit in FALLOC_FL_MODE_MASK the switch sees it as one
unknown mode and takes the default label. vz9 had no such switch and
ZERO_RANGE had no exclusivity check of its own, so the combination
passed, and ext4_fallocate() carried the whole mode down through
ext4_zero_range() into ext4_alloc_file_blocks(), where the flag is read.

Nothing is broken today, csd passes the flag on its own:

      fallocate(req->inplace_wr_fd,
                j->use_swapext ? FALLOC_FL_PREALLOCATE : 0, ...)

but zero range together with preallocation is no longer expressible.

3. Upstream has taken bit 0x80 in 6.17.

Commit 7bd43cc79cab ("fs: introduce FALLOC_FL_WRITE_ZEROES"), released
in v6.17, defines

      #define FALLOC_FL_WRITE_ZEROES          0x80

adds it to FALLOC_FL_MODE_MASK, and ext4 implements it. Upstream also
puts it in the group of that same switch which refuses
FALLOC_FL_KEEP_SIZE, next to collapse and insert range, while the mode
added here sits in the group that allows KEEP_SIZE.

On a rebase to v6.17 or later the two are one value in one switch.
Keeping both case labels does not build, the switch then has a
duplicate case value. Dropping either label silently changes the
meaning of a mode: drop ours and the feature quietly disappears again,
which is the very bug this patch is fixing; drop theirs and a
FALLOC_FL_WRITE_ZEROES request gets unwritten extents instead of zeroed
blocks, with KEEP_SIZE accepted where upstream refuses it.

Adopting the upstream mode instead of ours does not get us out of it.
It reaches ext4_alloc_file_blocks() through ext4_zero_range(), where

      if (len_lblk <= EXT_UNWRITTEN_MAX_LEN)
              flags |= EXT4_GET_BLOCKS_NO_NORMALIZE;

is unchanged, so it loses mballoc preallocation exactly like plain
fallocate and does nothing for us. It also does real zeroing plus
conversion to written, requires the device to report unmap write
zeroes, and refuses KEEP_SIZE.

So the way out is moving our flag to a free high bit, and yes, that
needs a coordinated userspace rebuild. csd_journal.c carries its own
copy of the value:

      #ifndef FALLOC_FL_PREALLOCATE
      #define FALLOC_FL_PREALLOCATE          0x80
      #endif

A rebuild against a header without the name would keep sending 0x80,
which on v6.17 is someone else's mode, so the renumbering has to remove
that block too.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel