Re: [PATCH v15 3/7] btrfs: add send stream v2 definitions

[Omar Sandoval <osandov@osandov.com>]

On Thu, May 19, 2022 at 06:07:49PM +0200, David Sterba wrote:
> On Wed, May 18, 2022 at 03:25:34PM -0700, Omar Sandoval wrote:
> > On Wed, May 18, 2022 at 11:00:03PM +0200, David Sterba wrote:
> > > On Mon, Apr 04, 2022 at 10:29:05AM -0700, Omar Sandoval wrote:
> > > > @@ -80,16 +84,20 @@ enum btrfs_send_cmd {
> > > >  	BTRFS_SEND_C_MAX_V1 = 22,
> > > >  
> > > >  	/* Version 2 */
> > > > -	BTRFS_SEND_C_MAX_V2 = 22,
> > > > +	BTRFS_SEND_C_FALLOCATE = 23,
> > > > +	BTRFS_SEND_C_SETFLAGS = 24,
> > > 
> > > Do you have patches that implement the fallocate modes and setflags? I
> > > don't see it in this patchset.
> > 
> > Nope, as discussed before, in order to keep the patch series managable,
> > this series adds the definitions and receive support for fallocate and
> > setflags, but leaves the send side to be implemented at a later time.
> > 
> > I implemented fallocate for send back in 2019:
> > https://github.com/osandov/linux/commits/btrfs-send-v2. It passed some
> > basic testing back then, but it'd need a big rebase and more testing.
> 
> The patches in the branch are partially cleanups and preparatory work,
> so at least avoiding sending the holes would be nice to have for v2 as
> it was one of the first bugs reported. The falllocate modes seem to be
> easy. The rest is about the versioning infrastructure that we already
> have merged.

I rebased the patches on this series:
https://github.com/osandov/linux/commits/btrfs-send-v2-redux. It passes
some basic testing, but it'll definitely need a lot of fstests.

> > > The setflags should be switched to
> > > something closer to the recent refactoring that unifies all the
> > > flags/attrs to fileattr. I have a prototype patch for that, comparing
> > > the inode flags in the same way as file mode, the tricky part is on the
> > > receive side how to apply them correctly. On the sending side it's
> > > simple though.
> > 
> > The way this series documents (and implements in receive)
> > BTRFS_SEND_C_SETFLAGS is that it's a simple call to FS_IOC_SETFLAGS with
> > given flags. I don't think this is affected by the change to fileattr,
> > unless I'm misunderstanding.
> 
> The SETFLAGS ioctls are obsolete and I don't want to make them part of
> the protocol defition because the bit namespace contains flags we don't
> have implemented or are not releated to anything in btrfs.
> 
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fs.h#L220
> 
> It's basically just naming and specifying what exactly is the value so
> we should pick the most recent interface name that superseded SETFLAGS
> and the XFLAGS.

This is the situation with FS_IOC_SETFLAGS, FS_IOC_FSSETXATTR, and
fileattr as I understand it. Please correct me if I'm wrong:

- FS_IOC_SETFLAGS originally came from ext4 and was added to Btrfs very
  early on (commit 6cbff00f4632 ("Btrfs: implement
  FS_IOC_GETFLAGS/SETFLAGS/GETVERSION")).
- FS_IOC_FSSETXATTR originally came from XFS and was added to Btrfs a
  few years ago (in commit 025f2121488e ("btrfs: add FS_IOC_FSSETXATTR
  ioctl")).
- The two ioctls allow setting some of the same flags (e.g., IMMUTABLE,
  APPEND), but some are only supported by SETFLAGS (e.g., NOCOW) and
  some are only supported by FSSETXATTR (none of these are supported by
  Btrfs, however).
- fileattr is a recent VFS interface that is used to implement those two
  ioctls. It basically passes through the arguments for whichever ioctl
  was called and translates the equivalent flags between the two ioctls.
  It is not a new UAPI and doesn't have its own set of flags.

Is there another new UAPI that I'm missing that obsoletes SETFLAGS?

I see your point about the irrelevant flags in SETFLAGS, however. Is
your suggestion to have our own send protocol-specific set of flags that
we translate to whatever ioctl we need to make?

> > This is in line with the other commands being straightforward system
> > calls, but it does mean that the sending side has to deal with the
> > complexities of an immutable or append-only file being modified between
> > incremental sends (by temporarily clearing the flag), and of inherited
> > flags (e.g., a COW file inside of a NOCOW directory).
> 
> Yeah the receiving side needs to understand the constraints of the
> flags, it has only the information about the final state and not the
> order in which the flags get applied.

If the sender only tells the receiver what the final flags are, then
yes, the receiver would need to deal with, e.g., temporarily clearing
and resetting flags. The way I envisioned it was that the sender would
instead send commands for those intermediate flag operations. E.g., if
the incremental send requires writing some data to a file that is
immutable in both the source and the parent subvolume, the sender could
send commands to: clear the immutable flag, write the data, set the
immutable flag. This is a lot like the orphan renaming that you
mentioned.

If we want to have receive handle the intermediate states instead, then
I would like to postpone SETFLAGS (or whatever we call it) to send
protocol v3, since it'll be very tricky to get right and we can't add it
to the protocol without having an implementation in the receiver.

On the other hand, if send takes care of the intermediate states and
receive just has to blindly apply the flags, then we can add SETFLAGS to
the protocol and receive now and implement it in send later. That is
exactly what this patch series does.

I'm fine with either of those paths forward, but I don't want to block
the compressed send/receive on SETFLAGS or fallocate.

Thanks,
Omar