* [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it
@ 2019-02-25 19:07 Goldwyn Rodrigues
2019-02-25 19:07 ` [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink Goldwyn Rodrigues
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Goldwyn Rodrigues @ 2019-02-25 19:07 UTC (permalink / raw)
To: linux-btrfs; +Cc: Goldwyn Rodrigues
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
io_ctl_init() memsets it to zero anyways. However, I presume the
memset was added to avoid the WARN_ON in io_ctl_init().
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
fs/btrfs/free-space-cache.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 74aa552f4793..c813378ebf08 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -3544,13 +3544,12 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root,
struct btrfs_fs_info *fs_info = root->fs_info;
struct btrfs_free_space_ctl *ctl = root->free_ino_ctl;
int ret;
- struct btrfs_io_ctl io_ctl;
+ struct btrfs_io_ctl io_ctl = {0};
bool release_metadata = true;
if (!btrfs_test_opt(fs_info, INODE_MAP_CACHE))
return 0;
- memset(&io_ctl, 0, sizeof(io_ctl));
ret = __btrfs_write_out_cache(root, inode, ctl, NULL, &io_ctl, trans);
if (!ret) {
/*
--
2.16.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink
2019-02-25 19:07 [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it Goldwyn Rodrigues
@ 2019-02-25 19:07 ` Goldwyn Rodrigues
2019-02-27 15:39 ` David Sterba
2019-02-25 19:07 ` [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range() Goldwyn Rodrigues
2019-02-27 15:47 ` [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it David Sterba
2 siblings, 1 reply; 11+ messages in thread
From: Goldwyn Rodrigues @ 2019-02-25 19:07 UTC (permalink / raw)
To: linux-btrfs; +Cc: Goldwyn Rodrigues
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
inode->i_op is initialized multiple times. Perform it once.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
fs/btrfs/inode.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5c349667c761..9b77fe994cbb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -10140,7 +10140,6 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry,
inode->i_op = &btrfs_symlink_inode_operations;
inode_nohighmem(inode);
- inode->i_mapping->a_ops = &btrfs_aops;
inode_set_bytes(inode, name_len);
btrfs_i_size_write(BTRFS_I(inode), name_len);
err = btrfs_update_inode(trans, root, inode);
--
2.16.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()
2019-02-25 19:07 [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it Goldwyn Rodrigues
2019-02-25 19:07 ` [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink Goldwyn Rodrigues
@ 2019-02-25 19:07 ` Goldwyn Rodrigues
2019-02-26 12:08 ` Filipe Manana
2019-02-27 15:47 ` [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it David Sterba
2 siblings, 1 reply; 11+ messages in thread
From: Goldwyn Rodrigues @ 2019-02-25 19:07 UTC (permalink / raw)
To: linux-btrfs; +Cc: Goldwyn Rodrigues
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
Moves code to make it more readable, so as locking and unlocking is
done in the same function.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
fs/btrfs/ioctl.c | 53 +++++++++++++++++++++--------------------------------
1 file changed, 21 insertions(+), 32 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9c8e1734429c..f0ae1af91ff3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3975,22 +3975,6 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
u64 wb_len;
int ret;
- if (!(remap_flags & REMAP_FILE_DEDUP)) {
- struct btrfs_root *root_out = BTRFS_I(inode_out)->root;
-
- if (btrfs_root_readonly(root_out))
- return -EROFS;
-
- if (file_in->f_path.mnt != file_out->f_path.mnt ||
- inode_in->i_sb != inode_out->i_sb)
- return -EXDEV;
- }
-
- if (same_inode)
- inode_lock(inode_in);
- else
- btrfs_double_inode_lock(inode_in, inode_out);
-
/*
* Now that the inodes are locked, we need to start writeback ourselves
* and can not rely on the writeback from the VFS's generic helper
@@ -4022,26 +4006,14 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
ret = btrfs_wait_ordered_range(inode_in, ALIGN_DOWN(pos_in, bs),
wb_len);
if (ret < 0)
- goto out_unlock;
+ return ret;
ret = btrfs_wait_ordered_range(inode_out, ALIGN_DOWN(pos_out, bs),
wb_len);
if (ret < 0)
- goto out_unlock;
+ return ret;
- ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
+ return generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
len, remap_flags);
- if (ret < 0 || *len == 0)
- goto out_unlock;
-
- return 0;
-
- out_unlock:
- if (same_inode)
- inode_unlock(inode_in);
- else
- btrfs_double_inode_unlock(inode_in, inode_out);
-
- return ret;
}
loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
@@ -4056,16 +4028,33 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
return -EINVAL;
+ if (!(remap_flags & REMAP_FILE_DEDUP)) {
+ struct btrfs_root *root_out = BTRFS_I(dst_inode)->root;
+
+ if (btrfs_root_readonly(root_out))
+ return -EROFS;
+
+ if (src_file->f_path.mnt != dst_file->f_path.mnt ||
+ src_inode->i_sb != dst_inode->i_sb)
+ return -EXDEV;
+ }
+
+ if (same_inode)
+ inode_lock(src_inode);
+ else
+ btrfs_double_inode_lock(src_inode, dst_inode);
+
ret = btrfs_remap_file_range_prep(src_file, off, dst_file, destoff,
&len, remap_flags);
if (ret < 0 || len == 0)
- return ret;
+ goto out_unlock;
if (remap_flags & REMAP_FILE_DEDUP)
ret = btrfs_extent_same(src_inode, off, len, dst_inode, destoff);
else
ret = btrfs_clone_files(dst_file, src_file, off, len, destoff);
+out_unlock:
if (same_inode)
inode_unlock(src_inode);
else
--
2.16.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()
2019-02-25 19:07 ` [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range() Goldwyn Rodrigues
@ 2019-02-26 12:08 ` Filipe Manana
2019-02-26 12:57 ` Goldwyn Rodrigues
0 siblings, 1 reply; 11+ messages in thread
From: Filipe Manana @ 2019-02-26 12:08 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
On Mon, Feb 25, 2019 at 7:08 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Moves code to make it more readable, so as locking and unlocking is
> done in the same function.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> fs/btrfs/ioctl.c | 53 +++++++++++++++++++++--------------------------------
> 1 file changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9c8e1734429c..f0ae1af91ff3 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3975,22 +3975,6 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
> u64 wb_len;
> int ret;
>
> - if (!(remap_flags & REMAP_FILE_DEDUP)) {
> - struct btrfs_root *root_out = BTRFS_I(inode_out)->root;
> -
> - if (btrfs_root_readonly(root_out))xfs_reflink_remap_prep
> - return -EROFS;
> -
> - if (file_in->f_path.mnt != file_out->f_path.mnt ||
> - inode_in->i_sb != inode_out->i_sb)
> - return -EXDEV;
> - }
Why move these checks?
The goal of the _prep function (both btrfs and vfs) is to have the
checks for all needed conditions in one place.
As for the lock/unlock, it follows the same pattern from xfs
(xfs_reflink_remap_prep and xfs_file_remap_range).
No complaints about changing this, I'm just neutral about it.
> -
> - if (same_inode)
> - inode_lock(inode_in);
> - else
> - btrfs_double_inode_lock(inode_in, inode_out);
> -
> /*
> * Now that the inodes are locked, we need to start writeback ourselves
> * and can not rely on the writeback from the VFS's generic helper
> @@ -4022,26 +4006,14 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
> ret = btrfs_wait_ordered_range(inode_in, ALIGN_DOWN(pos_in, bs),
> wb_len);
> if (ret < 0)
> - goto out_unlock;
> + return ret;
> ret = btrfs_wait_ordered_range(inode_out, ALIGN_DOWN(pos_out, bs),
> wb_len);
> if (ret < 0)
> - goto out_unlock;
> + return ret;
>
> - ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
> + return generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
> len, remap_flags);
> - if (ret < 0 || *len == 0)
> - goto out_unlock;
> -
> - return 0;
> -
> - out_unlock:
> - if (same_inode)
> - inode_unlock(inode_in);
> - else
> - btrfs_double_inode_unlock(inode_in, inode_out);
> -
> - return ret;
> }
>
> loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> @@ -4056,16 +4028,33 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> return -EINVAL;
>
> + if (!(remap_flags & REMAP_FILE_DEDUP)) {
> + struct btrfs_root *root_out = BTRFS_I(dst_inode)->root;
> +
> + if (btrfs_root_readonly(root_out))
> + return -EROFS;
> +
> + if (src_file->f_path.mnt != dst_file->f_path.mnt ||
> + src_inode->i_sb != dst_inode->i_sb)
> + return -EXDEV;
> + }
> +
> + if (same_inode)
> + inode_lock(src_inode);
> + else
> + btrfs_double_inode_lock(src_inode, dst_inode);
> +
> ret = btrfs_remap_file_range_prep(src_file, off, dst_file, destoff,
> &len, remap_flags);
> if (ret < 0 || len == 0)
> - return ret;
> + goto out_unlock;
>
> if (remap_flags & REMAP_FILE_DEDUP)
> ret = btrfs_extent_same(src_inode, off, len, dst_inode, destoff);
> else
> ret = btrfs_clone_files(dst_file, src_file, off, len, destoff);
>
> +out_unlock:
> if (same_inode)
> inode_unlock(src_inode);
> else
> --
> 2.16.4
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()
2019-02-26 12:08 ` Filipe Manana
@ 2019-02-26 12:57 ` Goldwyn Rodrigues
2019-02-27 14:52 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: Goldwyn Rodrigues @ 2019-02-26 12:57 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
On 12:08 26/02, Filipe Manana wrote:
> On Mon, Feb 25, 2019 at 7:08 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > Moves code to make it more readable, so as locking and unlocking is
> > done in the same function.
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> > fs/btrfs/ioctl.c | 53 +++++++++++++++++++++--------------------------------
> > 1 file changed, 21 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 9c8e1734429c..f0ae1af91ff3 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -3975,22 +3975,6 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
> > u64 wb_len;
> > int ret;
> >
> > - if (!(remap_flags & REMAP_FILE_DEDUP)) {
> > - struct btrfs_root *root_out = BTRFS_I(inode_out)->root;
> > -
> > - if (btrfs_root_readonly(root_out))xfs_reflink_remap_prep
> > - return -EROFS;
> > -
> > - if (file_in->f_path.mnt != file_out->f_path.mnt ||
> > - inode_in->i_sb != inode_out->i_sb)
> > - return -EXDEV;
> > - }
>
> Why move these checks?
> The goal of the _prep function (both btrfs and vfs) is to have the
> checks for all needed conditions in one place.
In the original flow, these checks were done without locks.
But I suppose they can be done with locks held as well.
>
> As for the lock/unlock, it follows the same pattern from xfs
> (xfs_reflink_remap_prep and xfs_file_remap_range).
> No complaints about changing this, I'm just neutral about it.
>
I just read the xfs code and yes it is similar. Locking and unlocking
in separate functions makes it difficult to read, especially
when it can be done in the same function.
> > -
> > - if (same_inode)
> > - inode_lock(inode_in);
> > - else
> > - btrfs_double_inode_lock(inode_in, inode_out);
> > -
> > /*
> > * Now that the inodes are locked, we need to start writeback ourselves
> > * and can not rely on the writeback from the VFS's generic helper
> > @@ -4022,26 +4006,14 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
> > ret = btrfs_wait_ordered_range(inode_in, ALIGN_DOWN(pos_in, bs),
> > wb_len);
> > if (ret < 0)
> > - goto out_unlock;
> > + return ret;
> > ret = btrfs_wait_ordered_range(inode_out, ALIGN_DOWN(pos_out, bs),
> > wb_len);
> > if (ret < 0)
> > - goto out_unlock;
> > + return ret;
> >
> > - ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
> > + return generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
> > len, remap_flags);
> > - if (ret < 0 || *len == 0)
> > - goto out_unlock;
> > -
> > - return 0;
> > -
> > - out_unlock:
> > - if (same_inode)
> > - inode_unlock(inode_in);
> > - else
> > - btrfs_double_inode_unlock(inode_in, inode_out);
> > -
> > - return ret;
> > }
> >
> > loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> > @@ -4056,16 +4028,33 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
> > if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> > return -EINVAL;
> >
> > + if (!(remap_flags & REMAP_FILE_DEDUP)) {
> > + struct btrfs_root *root_out = BTRFS_I(dst_inode)->root;
> > +
> > + if (btrfs_root_readonly(root_out))
> > + return -EROFS;
> > +
> > + if (src_file->f_path.mnt != dst_file->f_path.mnt ||
> > + src_inode->i_sb != dst_inode->i_sb)
> > + return -EXDEV;
> > + }
> > +
> > + if (same_inode)
> > + inode_lock(src_inode);
> > + else
> > + btrfs_double_inode_lock(src_inode, dst_inode);
> > +
> > ret = btrfs_remap_file_range_prep(src_file, off, dst_file, destoff,
> > &len, remap_flags);
> > if (ret < 0 || len == 0)
> > - return ret;
> > + goto out_unlock;
> >
> > if (remap_flags & REMAP_FILE_DEDUP)
> > ret = btrfs_extent_same(src_inode, off, len, dst_inode, destoff);
> > else
> > ret = btrfs_clone_files(dst_file, src_file, off, len, destoff);
> >
> > +out_unlock:
> > if (same_inode)
> > inode_unlock(src_inode);
> > else
> > --
> > 2.16.4
> >
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”
>
--
Goldwyn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()
2019-02-26 12:57 ` Goldwyn Rodrigues
@ 2019-02-27 14:52 ` David Sterba
2019-02-27 15:06 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2019-02-27 14:52 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: Filipe Manana, linux-btrfs
On Tue, Feb 26, 2019 at 06:57:17AM -0600, Goldwyn Rodrigues wrote:
> On 12:08 26/02, Filipe Manana wrote:
> > On Mon, Feb 25, 2019 at 7:08 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > >
> > > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > >
> > > Moves code to make it more readable, so as locking and unlocking is
> > > done in the same function.
> > >
> > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > > ---
> > > fs/btrfs/ioctl.c | 53 +++++++++++++++++++++--------------------------------
> > > 1 file changed, 21 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > index 9c8e1734429c..f0ae1af91ff3 100644
> > > --- a/fs/btrfs/ioctl.c
> > > +++ b/fs/btrfs/ioctl.c
> > > @@ -3975,22 +3975,6 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
> > > u64 wb_len;
> > > int ret;
> > >
> > > - if (!(remap_flags & REMAP_FILE_DEDUP)) {
> > > - struct btrfs_root *root_out = BTRFS_I(inode_out)->root;
> > > -
> > > - if (btrfs_root_readonly(root_out))xfs_reflink_remap_prep
> > > - return -EROFS;
> > > -
> > > - if (file_in->f_path.mnt != file_out->f_path.mnt ||
> > > - inode_in->i_sb != inode_out->i_sb)
> > > - return -EXDEV;
> > > - }
> >
> > Why move these checks?
> > The goal of the _prep function (both btrfs and vfs) is to have the
> > checks for all needed conditions in one place.
>
> In the original flow, these checks were done without locks.
> But I suppose they can be done with locks held as well.
The locking does not affect the above checks, so no problem here.
> >
> > As for the lock/unlock, it follows the same pattern from xfs
> > (xfs_reflink_remap_prep and xfs_file_remap_range).
> > No complaints about changing this, I'm just neutral about it.
> >
>
> I just read the xfs code and yes it is similar. Locking and unlocking
> in separate functions makes it difficult to read, especially
> when it can be done in the same function.
>
> > > -
> > > - if (same_inode)
> > > - inode_lock(inode_in);
> > > - else
> > > - btrfs_double_inode_lock(inode_in, inode_out);
> > > -
But removing the checks from here can't be done because there's inode
compatibility flag check done right here (in current code, since commit
500710d3b872) but it's not in this diff.
Otherwise there's a race with chatter, once fixed by
b5c40d598f5408bd0ca22dfffa82f03cd9433f23 "Btrfs: fix clone vs chattr
NODATASUM race".
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()
2019-02-27 14:52 ` David Sterba
@ 2019-02-27 15:06 ` David Sterba
2019-03-25 19:22 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2019-02-27 15:06 UTC (permalink / raw)
To: dsterba, Goldwyn Rodrigues, Filipe Manana, linux-btrfs
On Wed, Feb 27, 2019 at 03:52:42PM +0100, David Sterba wrote:
> > > > - struct btrfs_root *root_out = BTRFS_I(inode_out)->root;
> > > > -
> > > > - if (btrfs_root_readonly(root_out))xfs_reflink_remap_prep
> > > > - return -EROFS;
> > > > -
> > > > - if (file_in->f_path.mnt != file_out->f_path.mnt ||
> > > > - inode_in->i_sb != inode_out->i_sb)
> > > > - return -EXDEV;
> > > > - }
> > >
> > > Why move these checks?
> > > The goal of the _prep function (both btrfs and vfs) is to have the
> > > checks for all needed conditions in one place.
> >
> > In the original flow, these checks were done without locks.
> > But I suppose they can be done with locks held as well.
>
> The locking does not affect the above checks, so no problem here.
>
> > >
> > > As for the lock/unlock, it follows the same pattern from xfs
> > > (xfs_reflink_remap_prep and xfs_file_remap_range).
> > > No complaints about changing this, I'm just neutral about it.
> > >
> >
> > I just read the xfs code and yes it is similar. Locking and unlocking
> > in separate functions makes it difficult to read, especially
> > when it can be done in the same function.
> >
> > > > -
> > > > - if (same_inode)
> > > > - inode_lock(inode_in);
> > > > - else
> > > > - btrfs_double_inode_lock(inode_in, inode_out);
> > > > -
>
> But removing the checks from here can't be done because there's inode
> compatibility flag check done right here (in current code, since commit
> 500710d3b872) but it's not in this diff.
>
> Otherwise there's a race with chatter, once fixed by
> b5c40d598f5408bd0ca22dfffa82f03cd9433f23 "Btrfs: fix clone vs chattr
> NODATASUM race".
So the unchecked access to inode flags does not happen, I did not have a
clear picture of the change. The locks are only moved outside of _prep
to the caller.
The lock/unlock look better in the same function, they're close toe each
other on the same page, so ok from me.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink
2019-02-25 19:07 ` [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink Goldwyn Rodrigues
@ 2019-02-27 15:39 ` David Sterba
0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2019-02-27 15:39 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
On Mon, Feb 25, 2019 at 01:07:43PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> inode->i_op is initialized multiple times. Perform it once.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Leftover from 4779cc04248deff6
Reviewed-by: David Sterba <dsterba@suse.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it
2019-02-25 19:07 [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it Goldwyn Rodrigues
2019-02-25 19:07 ` [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink Goldwyn Rodrigues
2019-02-25 19:07 ` [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range() Goldwyn Rodrigues
@ 2019-02-27 15:47 ` David Sterba
2019-02-27 17:02 ` Goldwyn Rodrigues
2 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2019-02-27 15:47 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
On Mon, Feb 25, 2019 at 01:07:42PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> io_ctl_init() memsets it to zero anyways. However, I presume the
> memset was added to avoid the WARN_ON in io_ctl_init().
I don't see any WARN_ON in io_ctl_init, you probably mean
__btrfs_write_out_cache.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> fs/btrfs/free-space-cache.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 74aa552f4793..c813378ebf08 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -3544,13 +3544,12 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root,
> struct btrfs_fs_info *fs_info = root->fs_info;
> struct btrfs_free_space_ctl *ctl = root->free_ino_ctl;
> int ret;
> - struct btrfs_io_ctl io_ctl;
> + struct btrfs_io_ctl io_ctl = {0};
Doesn't this zero the bytes unconditionally? The memset below happens
only when the inode cache is on.
> bool release_metadata = true;
>
> if (!btrfs_test_opt(fs_info, INODE_MAP_CACHE))
> return 0;
>
> - memset(&io_ctl, 0, sizeof(io_ctl));
> ret = __btrfs_write_out_cache(root, inode, ctl, NULL, &io_ctl, trans);
> if (!ret) {
> /*
> --
> 2.16.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it
2019-02-27 15:47 ` [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it David Sterba
@ 2019-02-27 17:02 ` Goldwyn Rodrigues
0 siblings, 0 replies; 11+ messages in thread
From: Goldwyn Rodrigues @ 2019-02-27 17:02 UTC (permalink / raw)
To: dsterba, linux-btrfs
On 16:47 27/02, David Sterba wrote:
> On Mon, Feb 25, 2019 at 01:07:42PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > io_ctl_init() memsets it to zero anyways. However, I presume the
> > memset was added to avoid the WARN_ON in io_ctl_init().
>
> I don't see any WARN_ON in io_ctl_init, you probably mean
> __btrfs_write_out_cache.
Yes, __btrfs_write_out_cache.
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> > fs/btrfs/free-space-cache.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> > index 74aa552f4793..c813378ebf08 100644
> > --- a/fs/btrfs/free-space-cache.c
> > +++ b/fs/btrfs/free-space-cache.c
> > @@ -3544,13 +3544,12 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root,
> > struct btrfs_fs_info *fs_info = root->fs_info;
> > struct btrfs_free_space_ctl *ctl = root->free_ino_ctl;
> > int ret;
> > - struct btrfs_io_ctl io_ctl;
> > + struct btrfs_io_ctl io_ctl = {0};
>
> Doesn't this zero the bytes unconditionally? The memset below happens
> only when the inode cache is on.
Yes, but does it matter. I assumed assignment is faster than memset, but
a google search says gcc optimization is smart enough now. So, this patch
seems irrelevant.
--
Goldwyn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range()
2019-02-27 15:06 ` David Sterba
@ 2019-03-25 19:22 ` David Sterba
0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2019-03-25 19:22 UTC (permalink / raw)
To: dsterba, Goldwyn Rodrigues, Filipe Manana, linux-btrfs
On Wed, Feb 27, 2019 at 04:06:24PM +0100, David Sterba wrote:
> So the unchecked access to inode flags does not happen, I did not have a
> clear picture of the change. The locks are only moved outside of _prep
> to the caller.
>
> The lock/unlock look better in the same function, they're close toe each
> other on the same page, so ok from me.
I've applied the patch and only left the locking changes, as discussed.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-03-25 19:21 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-25 19:07 [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it Goldwyn Rodrigues
2019-02-25 19:07 ` [PATCH 2/3] btrfs: Initialize inode->i_op once in btrfs_symlink Goldwyn Rodrigues
2019-02-27 15:39 ` David Sterba
2019-02-25 19:07 ` [PATCH 3/3] btrfs: Perform locking/unlocking in btrfs_remap_file_range() Goldwyn Rodrigues
2019-02-26 12:08 ` Filipe Manana
2019-02-26 12:57 ` Goldwyn Rodrigues
2019-02-27 14:52 ` David Sterba
2019-02-27 15:06 ` David Sterba
2019-03-25 19:22 ` David Sterba
2019-02-27 15:47 ` [PATCH 1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it David Sterba
2019-02-27 17:02 ` Goldwyn Rodrigues
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.