Re: [PATCH v4 02/18] btrfs: merge PAGE_CLEAR_DIRTY and PAGE_SET_WRITEBACK into PAGE_START_WRITEBACK

From: Josef Bacik <josef@toxicpanda.com>
To: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v4 02/18] btrfs: merge PAGE_CLEAR_DIRTY and PAGE_SET_WRITEBACK into PAGE_START_WRITEBACK
Date: Tue, 19 Jan 2021 16:45:09 -0500	[thread overview]
Message-ID: <5dc997b3-66e1-a896-970c-a95491b332bb@toxicpanda.com> (raw)
In-Reply-To: <20210116071533.105780-3-wqu@suse.com>

On 1/16/21 2:15 AM, Qu Wenruo wrote:
> PAGE_CLEAR_DIRTY and PAGE_SET_WRITEBACK are two macros used in
> __process_pages_contig(), to inform the function to clear page dirty and
> then set page writeback.
> 
> However page write back and dirty are two conflict status (at least for
> sector size == PAGE_SIZE case), this means those two macros are always
> called together.
> 
> This means we can merge PAGE_CLEAR_DIRTY and PAGE_SET_WRITEBACK, into
> one macro, PAGE_START_WRITEBACK.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/extent_io.c |  4 ++--
>   fs/btrfs/extent_io.h | 12 ++++++------
>   fs/btrfs/inode.c     | 28 ++++++++++------------------
>   3 files changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 3442f1746683..a816ba4a8537 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1970,10 +1970,10 @@ static int __process_pages_contig(struct address_space *mapping,
>   			if (page_ops & PAGE_SET_PRIVATE2)
>   				SetPagePrivate2(pages[i]);
>   
> -			if (page_ops & PAGE_CLEAR_DIRTY)
> +			if (page_ops & PAGE_START_WRITEBACK) {
>   				clear_page_dirty_for_io(pages[i]);
> -			if (page_ops & PAGE_SET_WRITEBACK)
>   				set_page_writeback(pages[i]);
> +			}
>   			if (page_ops & PAGE_SET_ERROR)
>   				SetPageError(pages[i]);
>   			if (page_ops & PAGE_END_WRITEBACK)
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 19221095c635..bedf761a0300 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -35,12 +35,12 @@ enum {
>   
>   /* these are flags for __process_pages_contig */
>   #define PAGE_UNLOCK		(1 << 0)
> -#define PAGE_CLEAR_DIRTY	(1 << 1)
> -#define PAGE_SET_WRITEBACK	(1 << 2)
> -#define PAGE_END_WRITEBACK	(1 << 3)
> -#define PAGE_SET_PRIVATE2	(1 << 4)
> -#define PAGE_SET_ERROR		(1 << 5)
> -#define PAGE_LOCK		(1 << 6)
> +/* This one will clera page dirty and then set paeg writeback */
                     ^^^^^                         ^^^^
                     clear                         page

Sorry for some reason I missed this, then you can add my reviewed by from my 
previous reply.  Thanks,

Josef