Re: [PATCH v10 09/14] btrfs: add BTRFS_IOC_ENCODED_WRITE

Omar Sandoval
2021-08-20T18:00:01Z

On Fri, Aug 20, 2021 at 04:44:26PM +0300, Nikolay Borisov wrote:
>
>
> On 18.08.21 г. 0:06, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> >
> > The implementation resembles direct I/O: we have to flush any ordered
> > extents, invalidate the page cache, and do the io tree/delalloc/extent
> > map/ordered extent dance. From there, we can reuse the compression code
> > with a minor modification to distinguish the write from writeback. This
> > also creates inline extents when possible.
> >
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
>
> <snip>
>
> > * Add an entry indicating a block group or device which is pinned by a
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 7a0a9c752624..13a0a65c6a43 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -103,6 +103,8 @@ struct btrfs_ioctl_encoded_io_args_32 {
> >
> > #define BTRFS_IOC_ENCODED_READ_32 _IOR(BTRFS_IOCTL_MAGIC, 64, \
> > struct btrfs_ioctl_encoded_io_args_32)
> > +#define BTRFS_IOC_ENCODED_WRITE_32 _IOW(BTRFS_IOCTL_MAGIC, 64, \
> > + struct btrfs_ioctl_encoded_io_args_32)
> > #endif
> >
> > /* Mask out flags that are inappropriate for the given type of inode. */
> > @@ -4992,6 +4994,102 @@ static int btrfs_ioctl_encoded_read(struct file *file, void __user *argp,
> > return ret;
> > }
> >
> > +static int btrfs_ioctl_encoded_write(struct file *file, void __user *argp,
> > + bool compat)
> > +{
> > + struct btrfs_ioctl_encoded_io_args args;
> > + struct iovec iovstack[UIO_FASTIOV];
> > + struct iovec *iov = iovstack;
> > + struct iov_iter iter;
> > + loff_t pos;
> > + struct kiocb kiocb;
> > + ssize_t ret;
> > +
> > + if (!capable(CAP_SYS_ADMIN)) {
> > + ret = -EPERM;
> > + goto out_acct;
> > + }
> > +
> > + if (!(file->f_mode & FMODE_WRITE)) {
> > + ret = -EBADF;
> > + goto out_acct;
> > + }
> > +
> > + if (compat) {
> > +#if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
> > + struct btrfs_ioctl_encoded_io_args_32 args32;
> > +
> > + if (copy_from_user(&args32, argp, sizeof(args32))) {
> > + ret = -EFAULT;
> > + goto out_acct;
> > + }
> > + args.iov = compat_ptr(args32.iov);
> > + args.iovcnt = args.iovcnt;
> > + memcpy(&args.offset, &args32.offset,
> > + sizeof(args) -
> > + offsetof(struct btrfs_ioctl_encoded_io_args, offset));
> > +#else
> > + return -ENOTTY;
> > +#endif
> > + } else {
> > + if (copy_from_user(&args, argp, sizeof(args))) {
> > + ret = -EFAULT;
> > + goto out_acct;
> > + }
> > + }
> > +
> > + ret = -EINVAL;
> > + if (args.flags != 0)
> > + goto out_acct;
> > + if (memchr_inv(args.reserved, 0, sizeof(args.reserved)))
> > + goto out_acct;
> > + if (args.compression == BTRFS_ENCODED_IO_COMPRESSION_NONE &&
> > + args.encryption == BTRFS_ENCODED_IO_ENCRYPTION_NONE)
>
> Do you intend on supporting encrypted data writeout in the future, given
> that in btrfs_do_encoded_write EINVAL is returned if the data to be
> written is encrypted? If not then this check could be moved earlier to
> fail fast.

We probably want to support it at some point in the future, yes.

> > @@ -5138,9 +5236,13 @@ long btrfs_ioctl(struct file *file, unsigned int
> > return fsverity_ioctl_measure(file, argp);
> > case BTRFS_IOC_ENCODED_READ:
> > return btrfs_ioctl_encoded_read(file, argp, false);
> > + case BTRFS_IOC_ENCODED_WRITE:
> > + return btrfs_ioctl_encoded_write(file, argp, false);
> > #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
> > case BTRFS_IOC_ENCODED_READ_32:
> > return btrfs_ioctl_encoded_read(file, argp, true);
> > + case BTRFS_IOC_ENCODED_WRITE_32:
> > + return btrfs_ioctl_encoded_write(file, argp, true);
> > #endif
> > }
> >
> > diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> > index 550c34fa0e6d..180f302dee93 100644
> > --- a/fs/btrfs/ordered-data.c
> > +++ b/fs/btrfs/ordered-data.c
> > @@ -521,9 +521,15 @@ void btrfs_remove_ordered_extent(struct btrfs_inode *btrfs_inode,
> > spin_lock(&btrfs_inode->lock);
> > btrfs_mod_outstanding_extents(btrfs_inode, -1);
> > spin_unlock(&btrfs_inode->lock);
> > - if (root != fs_info->tree_root)
> > - btrfs_delalloc_release_metadata(btrfs_inode, entry->num_bytes,
> > - false);
> > + if (root != fs_info->tree_root) {
> > + u64 release;
> > +
> > + if (test_bit(BTRFS_ORDERED_ENCODED, &entry->flags))
> > + release = entry->disk_num_bytes;
> > + else
> > + release = entry->num_bytes;
> > + btrfs_delalloc_release_metadata(btrfs_inode, release, false);
> > + }
> >
> > percpu_counter_add_batch(&fs_info->ordered_bytes, -entry->num_bytes,
> > fs_info->delalloc_batch);
> > diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> > index 0feb0c29839e..04588ccad34c 100644
> > --- a/fs/btrfs/ordered-data.h
> > +++ b/fs/btrfs/ordered-data.h
> > @@ -74,6 +74,8 @@ enum {
> > BTRFS_ORDERED_LOGGED_CSUM,
> > /* We wait for this extent to complete in the current transaction */
> > BTRFS_ORDERED_PENDING,
> > + /* RWF_ENCODED I/O */
>
> nit: RWF_ENCODED is no longer, we simply have ioctl-based encoded io. So
> this needs to be renamed to avoid confusion for people not necessarily
> faimilar with the development history of the feature.

Good catch, thanks.