Re: [PATCH 5/5] btrfs: add no_mtime flag to btrfs-extent-same

Mark Fasheh
2015-06-23T17:11:27Z

On Tue, Jun 23, 2015 at 05:11:56PM +0200, David Sterba wrote:
> On Mon, Jun 22, 2015 at 03:47:42PM -0700, Mark Fasheh wrote:
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -87,7 +87,8 @@ struct btrfs_ioctl_received_subvol_args_32 {
> >
> >
> > static int btrfs_clone(struct inode *src, struct inode *inode,
> > - u64 off, u64 olen, u64 olen_aligned, u64 destoff);
> > + u64 off, u64 olen, u64 olen_aligned, u64 destoff,
> > + int no_mtime);
>
> Please make it 'flags' and pass the verified flags directly.

Sure.

> > /* Mask out flags that are inappropriate for the given type of inode. */
> > static inline __u32 btrfs_mask_flags(umode_t mode, __u32 flags)
> > @@ -2974,7 +2975,7 @@ static int extent_same_check_offsets(struct inode *inode, u64 off, u64 *plen,
> > }
> >
> > static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
> > - struct inode *dst, u64 dst_loff)
> > + struct inode *dst, u64 dst_loff, int no_mtime)
> > {
> > int ret;
> > u64 len = olen;
> > @@ -3054,7 +3055,8 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
> > /* pass original length for comparison so we stay within i_size */
> > ret = btrfs_cmp_data(src, loff, dst, dst_loff, olen, &cmp);
> > if (ret == 0)
> > - ret = btrfs_clone(src, dst, loff, olen, len, dst_loff);
> > + ret = btrfs_clone(src, dst, loff, olen, len, dst_loff,
> > + no_mtime);
> >
> > if (same_inode)
> > unlock_extent(&BTRFS_I(src)->io_tree, same_lock_start,
> > @@ -3088,6 +3090,7 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
> > u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> > bool is_admin = capable(CAP_SYS_ADMIN);
> > u16 count;
> > + int no_mtime = 0;
> >
> > if (!(file->f_mode & FMODE_READ))
> > return -EINVAL;
> > @@ -3139,6 +3142,12 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
> > if (!S_ISREG(src->i_mode))
> > goto out;
> >
> > + ret = -EINVAL;
> > + if (same->flags & ~BTRFS_SAME_FLAGS)
> > + goto out;
> > + if (same->flags & BTRFS_SAME_NO_MTIME)
> > + no_mtime = 1;
> > +
> > /* pre-format output fields to sane values */
> > for (i = 0; i < count; i++) {
> > same->info[i].bytes_deduped = 0ULL;
> > @@ -3164,7 +3173,8 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
> > info->status = -EACCES;
> > } else {
> > info->status = btrfs_extent_same(src, off, len, dst,
> > - info->logical_offset);
> > + info->logical_offset,
> > + no_mtime);
> > if (info->status == 0)
> > info->bytes_deduped += len;
> > }
> > @@ -3219,13 +3229,17 @@ static int clone_finish_inode_update(struct btrfs_trans_handle *trans,
> > struct inode *inode,
> > u64 endoff,
> > const u64 destoff,
> > - const u64 olen)
> > + const u64 olen,
> > + int no_mtime)
> > {
> > struct btrfs_root *root = BTRFS_I(inode)->root;
> > int ret;
> >
> > inode_inc_iversion(inode);
> > - inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> > + if (no_mtime)
> > + inode->i_ctime = CURRENT_TIME;
> > + else
> > + inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> > /*
> > * We round up to the block size at eof when determining which
> > * extents to clone above, but shouldn't round up the file size.
> > @@ -3316,7 +3330,7 @@ static void clone_update_extent_map(struct inode *inode,
> > */
> > static int btrfs_clone(struct inode *src, struct inode *inode,
> > const u64 off, const u64 olen, const u64 olen_aligned,
> > - const u64 destoff)
> > + const u64 destoff, int no_mtime)
> > {
> > struct btrfs_root *root = BTRFS_I(inode)->root;
> > struct btrfs_path *path = NULL;
> > @@ -3640,7 +3654,7 @@ process_slot:
> > root->sectorsize);
> > ret = clone_finish_inode_update(trans, inode,
> > last_dest_end,
> > - destoff, olen);
> > + destoff, olen, no_mtime);
> > if (ret)
> > goto out;
> > if (new_key.offset + datal >= destoff + len)
> > @@ -3678,7 +3692,7 @@ process_slot:
> > clone_update_extent_map(inode, trans, NULL, last_dest_end,
> > destoff + len - last_dest_end);
> > ret = clone_finish_inode_update(trans, inode, destoff + len,
> > - destoff, olen);
> > + destoff, olen, no_mtime);
> > }
> >
> > out:
> > @@ -3808,7 +3822,7 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
> > btrfs_double_extent_lock(src, off, inode, destoff, len);
> > }
> >
> > - ret = btrfs_clone(src, inode, off, olen, len, destoff);
> > + ret = btrfs_clone(src, inode, off, olen, len, destoff, 0);
> >
> > if (same_inode) {
> > u64 lock_start = min_t(u64, off, destoff);
> > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> > index b6dec05..beeb51c 100644
> > --- a/include/uapi/linux/btrfs.h
> > +++ b/include/uapi/linux/btrfs.h
>
> > +#define BTRFS_SAME_NO_MTIME 0x1
> > +#define BTRFS_SAME_FLAGS (BTRFS_SAME_NO_MTIME)
>
> The naming scheme used eg. for the send flags:
>
> BTRFS_SEND_FLAG_MASK
> BTRFS_SEND_FLAG_NO_FILE_DATA
>
> So I suggest to follow it:
>
> BTRFS_SAME_FLAG_MASK
> BTRFS_SAME_FLAG_NO_MTIME
>
> Though I'd rather see it named it with BTRFS_EXTENT_SAME_ prefix so it
> (almost ...) matches the ioctl name (and is greppable).
>
> This is going to be in a public interface so the naming is important.

Sounds good, I'll fix up the patch with your suggestions and resend another
round shortly. Btw thanks for all the reviews David, it is appreciated!
--Mark

--
Mark Fasheh