Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Naohiro Aota <Naohiro.Aota@wdc.com>
To:     Josef Bacik <josef@toxicpanda.com>
CC:     "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
        David Sterba <dsterba@suse.com>, Chris Mason <clm@fb.com>,
        Qu Wenruo <wqu@suse.com>, Nikolay Borisov <nborisov@suse.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Hannes Reinecke <hare@suse.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Damien Le Moal <Damien.LeMoal@wdc.com>,
        =?iso-8859-1?Q?Matias_Bj=F8rling?= <mb@lightnvm.io>,
        Johannes Thumshirn <jthumshirn@suse.de>,
        Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCH 14/19] btrfs: redirty released extent buffers in
 sequential BGs
Thread-Topic: [PATCH 14/19] btrfs: redirty released extent buffers in
 sequential BGs
Thread-Index: AQHVHTKP/a9sOH6tTEWoQyojzmYTGQ==
Date:   Tue, 18 Jun 2019 09:09:34 +0000
Message-ID: <SN6PR04MB523180783547B7CEC55285968CEA0@SN6PR04MB5231.namprd04.prod.outlook.com>
References: <20190607131025.31996-1-naohiro.aota@wdc.com>
 <20190607131025.31996-15-naohiro.aota@wdc.com>
 <20190613142408.p3ra5urczrzgqr2q@MacBook-Pro-91.local>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c104ad92-f80d-46dc-f8b0-08d6f3ccaea4
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jun 2019 09:09:34.7469
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Naohiro.Aota1@wdc.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR04MB5118
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2019/06/13 23:24, Josef Bacik wrote:=0A=
> On Fri, Jun 07, 2019 at 10:10:20PM +0900, Naohiro Aota wrote:=0A=
>> Tree manipulating operations like merging nodes often release=0A=
>> once-allocated tree nodes. Btrfs cleans such nodes so that pages in the=
=0A=
>> node are not uselessly written out. On HMZONED drives, however, such=0A=
>> optimization blocks the following IOs as the cancellation of the write o=
ut=0A=
>> of the freed blocks breaks the sequential write sequence expected by the=
=0A=
>> device.=0A=
>>=0A=
>> This patch introduces a list of clean extent buffers that have been=0A=
>> released in a transaction. Btrfs consult the list before writing out and=
=0A=
>> waiting for the IOs, and it redirties a buffer if 1) it's in sequential =
BG,=0A=
>> 2) it's in un-submit range, and 3) it's not under IO. Thus, such buffers=
=0A=
>> are marked for IO in btrfs_write_and_wait_transaction() to send proper b=
ios=0A=
>> to the disk.=0A=
>>=0A=
>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>=0A=
>> ---=0A=
>>   fs/btrfs/disk-io.c     | 27 ++++++++++++++++++++++++---=0A=
>>   fs/btrfs/extent_io.c   |  1 +=0A=
>>   fs/btrfs/extent_io.h   |  2 ++=0A=
>>   fs/btrfs/transaction.c | 35 +++++++++++++++++++++++++++++++++++=0A=
>>   fs/btrfs/transaction.h |  3 +++=0A=
>>   5 files changed, 65 insertions(+), 3 deletions(-)=0A=
>>=0A=
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c=0A=
>> index 6651986da470..c6147fce648f 100644=0A=
>> --- a/fs/btrfs/disk-io.c=0A=
>> +++ b/fs/btrfs/disk-io.c=0A=
>> @@ -535,7 +535,9 @@ static int csum_dirty_buffer(struct btrfs_fs_info *f=
s_info, struct page *page)=0A=
>>   	if (csum_tree_block(eb, result))=0A=
>>   		return -EINVAL;=0A=
>>   =0A=
>> -	if (btrfs_header_level(eb))=0A=
>> +	if (test_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags))=0A=
>> +		ret =3D 0;=0A=
>> +	else if (btrfs_header_level(eb))=0A=
>>   		ret =3D btrfs_check_node(eb);=0A=
>>   	else=0A=
>>   		ret =3D btrfs_check_leaf_full(eb);=0A=
>> @@ -1115,10 +1117,20 @@ struct extent_buffer *read_tree_block(struct btr=
fs_fs_info *fs_info, u64 bytenr,=0A=
>>   void btrfs_clean_tree_block(struct extent_buffer *buf)=0A=
>>   {=0A=
>>   	struct btrfs_fs_info *fs_info =3D buf->fs_info;=0A=
>> -	if (btrfs_header_generation(buf) =3D=3D=0A=
>> -	    fs_info->running_transaction->transid) {=0A=
>> +	struct btrfs_transaction *cur_trans =3D fs_info->running_transaction;=
=0A=
>> +=0A=
>> +	if (btrfs_header_generation(buf) =3D=3D cur_trans->transid) {=0A=
>>   		btrfs_assert_tree_locked(buf);=0A=
>>   =0A=
>> +		if (btrfs_fs_incompat(fs_info, HMZONED) &&=0A=
>> +		    list_empty(&buf->release_list)) {=0A=
>> +			atomic_inc(&buf->refs);=0A=
>> +			spin_lock(&cur_trans->releasing_ebs_lock);=0A=
>> +			list_add_tail(&buf->release_list,=0A=
>> +				      &cur_trans->releasing_ebs);=0A=
>> +			spin_unlock(&cur_trans->releasing_ebs_lock);=0A=
>> +		}=0A=
>> +=0A=
>>   		if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &buf->bflags)) {=0A=
>>   			percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,=0A=
>>   						 -buf->len,=0A=
>> @@ -4533,6 +4545,15 @@ void btrfs_cleanup_one_transaction(struct btrfs_t=
ransaction *cur_trans,=0A=
>>   	btrfs_destroy_pinned_extent(fs_info,=0A=
>>   				    fs_info->pinned_extents);=0A=
>>   =0A=
>> +	while (!list_empty(&cur_trans->releasing_ebs)) {=0A=
>> +		struct extent_buffer *eb;=0A=
>> +=0A=
>> +		eb =3D list_first_entry(&cur_trans->releasing_ebs,=0A=
>> +				      struct extent_buffer, release_list);=0A=
>> +		list_del_init(&eb->release_list);=0A=
>> +		free_extent_buffer(eb);=0A=
>> +	}=0A=
>> +=0A=
>>   	cur_trans->state =3DTRANS_STATE_COMPLETED;=0A=
>>   	wake_up(&cur_trans->commit_wait);=0A=
>>   }=0A=
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c=0A=
>> index 13fca7bfc1f2..c73c69e2bef4 100644=0A=
>> --- a/fs/btrfs/extent_io.c=0A=
>> +++ b/fs/btrfs/extent_io.c=0A=
>> @@ -4816,6 +4816,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_inf=
o, u64 start,=0A=
>>   	init_waitqueue_head(&eb->read_lock_wq);=0A=
>>   =0A=
>>   	btrfs_leak_debug_add(&eb->leak_list, &buffers);=0A=
>> +	INIT_LIST_HEAD(&eb->release_list);=0A=
>>   =0A=
>>   	spin_lock_init(&eb->refs_lock);=0A=
>>   	atomic_set(&eb->refs, 1);=0A=
>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h=0A=
>> index aa18a16a6ed7..2987a01f84f9 100644=0A=
>> --- a/fs/btrfs/extent_io.h=0A=
>> +++ b/fs/btrfs/extent_io.h=0A=
>> @@ -58,6 +58,7 @@ enum {=0A=
>>   	EXTENT_BUFFER_IN_TREE,=0A=
>>   	/* write IO error */=0A=
>>   	EXTENT_BUFFER_WRITE_ERR,=0A=
>> +	EXTENT_BUFFER_NO_CHECK,=0A=
>>   };=0A=
>>   =0A=
>>   /* these are flags for __process_pages_contig */=0A=
>> @@ -186,6 +187,7 @@ struct extent_buffer {=0A=
>>   	 */=0A=
>>   	wait_queue_head_t read_lock_wq;=0A=
>>   	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];=0A=
>> +	struct list_head release_list;=0A=
>>   #ifdef CONFIG_BTRFS_DEBUG=0A=
>>   	atomic_t spinning_writers;=0A=
>>   	atomic_t spinning_readers;=0A=
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c=0A=
>> index 3f6811cdf803..ded40ad75419 100644=0A=
>> --- a/fs/btrfs/transaction.c=0A=
>> +++ b/fs/btrfs/transaction.c=0A=
>> @@ -236,6 +236,8 @@ static noinline int join_transaction(struct btrfs_fs=
_info *fs_info,=0A=
>>   	spin_lock_init(&cur_trans->dirty_bgs_lock);=0A=
>>   	INIT_LIST_HEAD(&cur_trans->deleted_bgs);=0A=
>>   	spin_lock_init(&cur_trans->dropped_roots_lock);=0A=
>> +	INIT_LIST_HEAD(&cur_trans->releasing_ebs);=0A=
>> +	spin_lock_init(&cur_trans->releasing_ebs_lock);=0A=
>>   	list_add_tail(&cur_trans->list, &fs_info->trans_list);=0A=
>>   	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,=0A=
>>   			IO_TREE_TRANS_DIRTY_PAGES, fs_info->btree_inode);=0A=
>> @@ -2219,7 +2221,31 @@ int btrfs_commit_transaction(struct btrfs_trans_h=
andle *trans)=0A=
>>   =0A=
>>   	wake_up(&fs_info->transaction_wait);=0A=
>>   =0A=
>> +	if (btrfs_fs_incompat(fs_info, HMZONED)) {=0A=
>> +		struct extent_buffer *eb;=0A=
>> +=0A=
>> +		list_for_each_entry(eb, &cur_trans->releasing_ebs,=0A=
>> +				    release_list) {=0A=
>> +			struct btrfs_block_group_cache *cache;=0A=
>> +=0A=
>> +			cache =3D btrfs_lookup_block_group(fs_info, eb->start);=0A=
>> +			if (!cache)=0A=
>> +				continue;=0A=
>> +			mutex_lock(&cache->submit_lock);=0A=
>> +			if (cache->alloc_type =3D=3D BTRFS_ALLOC_SEQ &&=0A=
>> +			    cache->submit_offset <=3D eb->start &&=0A=
>> +			    !extent_buffer_under_io(eb)) {=0A=
>> +				set_extent_buffer_dirty(eb);=0A=
>> +				cache->space_info->bytes_readonly +=3D eb->len;=0A=
> =0A=
> Huh?=0A=
> =0A=
=0A=
I'm tracking once allocated then freed region in "space_info->bytes_readonl=
y".=0A=
As I wrote in the other reply, I can add and use "space_info->bytes_zone_un=
available" instead.=0A=
=0A=
>> +				set_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags);=0A=
>> +			}=0A=
>> +			mutex_unlock(&cache->submit_lock);=0A=
>> +			btrfs_put_block_group(cache);=0A=
>> +		}=0A=
>> +	}=0A=
>> +=0A=
> =0A=
> Helper here please.=0A=
>>   	ret =3D btrfs_write_and_wait_transaction(trans);=0A=
>> +=0A=
>>   	if (ret) {=0A=
>>   		btrfs_handle_fs_error(fs_info, ret,=0A=
>>   				      "Error while writing out transaction");=0A=
>> @@ -2227,6 +2253,15 @@ int btrfs_commit_transaction(struct btrfs_trans_h=
andle *trans)=0A=
>>   		goto scrub_continue;=0A=
>>   	}=0A=
>>   =0A=
>> +	while (!list_empty(&cur_trans->releasing_ebs)) {=0A=
>> +		struct extent_buffer *eb;=0A=
>> +=0A=
>> +		eb =3D list_first_entry(&cur_trans->releasing_ebs,=0A=
>> +				      struct extent_buffer, release_list);=0A=
>> +		list_del_init(&eb->release_list);=0A=
>> +		free_extent_buffer(eb);=0A=
>> +	}=0A=
>> +=0A=
> =0A=
> Another helper, and also can't we release eb's above that we didn't need =
to=0A=
> re-mark dirty?  Thanks,=0A=
> =0A=
> Josef=0A=
> =0A=
=0A=
hm, we can do so. I'll change the code in the next version.=0A=
Thanks,=0A=