diff mbox series

[v2] btrfs: Remove force argument from run_delalloc_nocow()

Message ID 20210225205822.mgx5ei3tzcqmlts6@fiona (mailing list archive)
State New, archived
Headers show
Series [v2] btrfs: Remove force argument from run_delalloc_nocow() | expand

Commit Message

Goldwyn Rodrigues Feb. 25, 2021, 8:58 p.m. UTC 
force_nocow can be calculated by btrfs_inode and does not need to be
passed as an argument.

This simplifies run_delalloc_nocow() call from btrfs_run_delalloc_range()
since the decision whether the extent is cow'd or not can be derived
from need_force_cow(). Since BTRFS_INODE_NODATACOW and
BTRFS_INODE_PREALLOC flags are checked in need_force_cow(), there is
no need to check it again.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Change since v1:
 - Kept need_force_cow() as it is

---
 fs/btrfs/inode.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Amy Parker Feb. 25, 2021, 11:22 p.m. UTC | #1 
On Thu, Feb 25, 2021 at 1:04 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> force_nocow can be calculated by btrfs_inode and does not need to be
> passed as an argument.
>
> This simplifies run_delalloc_nocow() call from btrfs_run_delalloc_range()
> since the decision whether the extent is cow'd or not can be derived
> from need_force_cow(). Since BTRFS_INODE_NODATACOW and
> BTRFS_INODE_PREALLOC flags are checked in need_force_cow(), there is
> no need to check it again.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Change since v1:
>  - Kept need_force_cow() as it is
>
> ---
>  fs/btrfs/inode.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4f2f1e932751..e5dd8d7ef0c8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1516,7 +1516,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
>  static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>                                        struct page *locked_page,
>                                        const u64 start, const u64 end,
> -                                      int *page_started, int force,
> +                                      int *page_started,
>                                        unsigned long *nr_written)
>  {
>         struct btrfs_fs_info *fs_info = inode->root->fs_info;
> @@ -1530,6 +1530,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>         u64 ino = btrfs_ino(inode);
>         bool nocow = false;
>         u64 disk_bytenr = 0;
> +       bool force = inode->flags & BTRFS_INODE_NODATACOW;
>
>         path = btrfs_alloc_path();
>         if (!path) {
> @@ -1891,17 +1892,12 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>                 struct writeback_control *wbc)
>  {
>         int ret;
> -       int force_cow = need_force_cow(inode, start, end);
>         const bool zoned = btrfs_is_zoned(inode->root->fs_info);
>
> -       if (inode->flags & BTRFS_INODE_NODATACOW && !force_cow) {
> +       if (!need_force_cow(inode, start, end)) {
>                 ASSERT(!zoned);
>                 ret = run_delalloc_nocow(inode, locked_page, start, end,
> -                                        page_started, 1, nr_written);
> -       } else if (inode->flags & BTRFS_INODE_PREALLOC && !force_cow) {
> -               ASSERT(!zoned);
> -               ret = run_delalloc_nocow(inode, locked_page, start, end,
> -                                        page_started, 0, nr_written);
> +                                        page_started, nr_written);
>         } else if (!inode_can_compress(inode) ||
>                    !inode_need_compress(inode, start, end)) {
>                 if (zoned)
> --
> 2.30.1
>
>
> --
> Goldwyn

Acked-by: Amy Parker <enbyamy@gmail.com>

Not super familiar with the btrfs inode implementation - but from what
I know of it and what I can read here it looks good to me.

   -Amy IP
David Sterba Feb. 26, 2021, 5:14 p.m. UTC | #2 
On Thu, Feb 25, 2021 at 02:58:22PM -0600, Goldwyn Rodrigues wrote:
> force_nocow can be calculated by btrfs_inode and does not need to be
> passed as an argument.
> 
> This simplifies run_delalloc_nocow() call from btrfs_run_delalloc_range()
> since the decision whether the extent is cow'd or not can be derived
> from need_force_cow(). Since BTRFS_INODE_NODATACOW and
> BTRFS_INODE_PREALLOC flags are checked in need_force_cow(), there is
> no need to check it again.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Change since v1:
>  - Kept need_force_cow() as it is

Added to misc-next, thanks.

> ---
>  fs/btrfs/inode.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4f2f1e932751..e5dd8d7ef0c8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1516,7 +1516,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
>  static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>  				       struct page *locked_page,
>  				       const u64 start, const u64 end,
> -				       int *page_started, int force,
> +				       int *page_started,
>  				       unsigned long *nr_written)
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> @@ -1530,6 +1530,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
>  	u64 ino = btrfs_ino(inode);
>  	bool nocow = false;
>  	u64 disk_bytenr = 0;
> +	bool force = inode->flags & BTRFS_INODE_NODATACOW;
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {
> @@ -1891,17 +1892,12 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>  		struct writeback_control *wbc)
>  {
>  	int ret;
> -	int force_cow = need_force_cow(inode, start, end);
>  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
>  
> -	if (inode->flags & BTRFS_INODE_NODATACOW && !force_cow) {
> +	if (!need_force_cow(inode, start, end)) {

We may want to reverse the logic and naming so it says
'need_force_nocow', right now it's "we don't need to force COW, so let's
do NOCOW", because COW is the default and the condition should pick the
exceptional case.

>  		ASSERT(!zoned);
>  		ret = run_delalloc_nocow(inode, locked_page, start, end,
> -					 page_started, 1, nr_written);
> -	} else if (inode->flags & BTRFS_INODE_PREALLOC && !force_cow) {
> -		ASSERT(!zoned);
> -		ret = run_delalloc_nocow(inode, locked_page, start, end,
> -					 page_started, 0, nr_written);
> +					 page_started, nr_written);
>  	} else if (!inode_can_compress(inode) ||
>  		   !inode_need_compress(inode, start, end)) {
>  		if (zoned)
> -- 
> 2.30.1
> 
> 
> -- 
> Goldwyn
Goldwyn Rodrigues Feb. 26, 2021, 6:47 p.m. UTC | #3 
On 18:14 26/02, David Sterba wrote:
> On Thu, Feb 25, 2021 at 02:58:22PM -0600, Goldwyn Rodrigues wrote:
> > force_nocow can be calculated by btrfs_inode and does not need to be
> > passed as an argument.
> > 
> > This simplifies run_delalloc_nocow() call from btrfs_run_delalloc_range()
> > since the decision whether the extent is cow'd or not can be derived
> > from need_force_cow(). Since BTRFS_INODE_NODATACOW and
> > BTRFS_INODE_PREALLOC flags are checked in need_force_cow(), there is
> > no need to check it again.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Change since v1:
> >  - Kept need_force_cow() as it is
> 
> Added to misc-next, thanks.
> 
> > ---
> >  fs/btrfs/inode.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 4f2f1e932751..e5dd8d7ef0c8 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -1516,7 +1516,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
> >  static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> >  				       struct page *locked_page,
> >  				       const u64 start, const u64 end,
> > -				       int *page_started, int force,
> > +				       int *page_started,
> >  				       unsigned long *nr_written)
> >  {
> >  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> > @@ -1530,6 +1530,7 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> >  	u64 ino = btrfs_ino(inode);
> >  	bool nocow = false;
> >  	u64 disk_bytenr = 0;
> > +	bool force = inode->flags & BTRFS_INODE_NODATACOW;
> >  
> >  	path = btrfs_alloc_path();
> >  	if (!path) {
> > @@ -1891,17 +1892,12 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> >  		struct writeback_control *wbc)
> >  {
> >  	int ret;
> > -	int force_cow = need_force_cow(inode, start, end);
> >  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
> >  
> > -	if (inode->flags & BTRFS_INODE_NODATACOW && !force_cow) {
> > +	if (!need_force_cow(inode, start, end)) {
> 
> We may want to reverse the logic and naming so it says
> 'need_force_nocow', right now it's "we don't need to force COW, so let's
> do NOCOW", because COW is the default and the condition should pick the
> exceptional case.

Calling it need_force_nocow() or should_nocow() will result in the same
concept as previous patch where EXTENT_DEFRAG range would need to be
checked before checking inode flags BTRFS_INODE_NODATACOW and
BTRFS_INODE_PREALLOC. That's because EXTENT_DEFRAG in range
should result in a COW sequence immaterial of what CoW sequence the
inode flags say. Isn't it?

Maybe something like this would keep inode flags checks earlier than
test_range_bit():

static inline bool should_nocow(struct btrfs_inode *inode, u64 start, u64 end)
{
        if (inode->flags & (BTRFS_INODE_NODATACOW | BTRFS_INODE_PREALLOC)) {
                if (inode->defrag_bytes &&
                    test_range_bit(&inode->io_tree, start, end, EXTENT_DEFRAG, 0, NULL))
                        return false;
                return true;
        }
        return false;
}



> 
> >  		ASSERT(!zoned);
> >  		ret = run_delalloc_nocow(inode, locked_page, start, end,
> > -					 page_started, 1, nr_written);
> > -	} else if (inode->flags & BTRFS_INODE_PREALLOC && !force_cow) {
> > -		ASSERT(!zoned);
> > -		ret = run_delalloc_nocow(inode, locked_page, start, end,
> > -					 page_started, 0, nr_written);
> > +					 page_started, nr_written);
> >  	} else if (!inode_can_compress(inode) ||
> >  		   !inode_need_compress(inode, start, end)) {
> >  		if (zoned)
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4f2f1e932751..e5dd8d7ef0c8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1516,7 +1516,7 @@  static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
 static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 				       struct page *locked_page,
 				       const u64 start, const u64 end,
-				       int *page_started, int force,
+				       int *page_started,
 				       unsigned long *nr_written)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
@@ -1530,6 +1530,7 @@  static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
 	u64 ino = btrfs_ino(inode);
 	bool nocow = false;
 	u64 disk_bytenr = 0;
+	bool force = inode->flags & BTRFS_INODE_NODATACOW;
 
 	path = btrfs_alloc_path();
 	if (!path) {
@@ -1891,17 +1892,12 @@  int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 		struct writeback_control *wbc)
 {
 	int ret;
-	int force_cow = need_force_cow(inode, start, end);
 	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
 
-	if (inode->flags & BTRFS_INODE_NODATACOW && !force_cow) {
+	if (!need_force_cow(inode, start, end)) {
 		ASSERT(!zoned);
 		ret = run_delalloc_nocow(inode, locked_page, start, end,
-					 page_started, 1, nr_written);
-	} else if (inode->flags & BTRFS_INODE_PREALLOC && !force_cow) {
-		ASSERT(!zoned);
-		ret = run_delalloc_nocow(inode, locked_page, start, end,
-					 page_started, 0, nr_written);
+					 page_started, nr_written);
 	} else if (!inode_can_compress(inode) ||
 		   !inode_need_compress(inode, start, end)) {
 		if (zoned)