[PATCH 1/2] btrfs: Remove invalid null checks from btrfs_cleanup_dirty_bgs

[PATCH 1/2] btrfs: Remove invalid null checks from btrfs_cleanup_dirty_bgs

[Nikolay Borisov]

list_first_entry is essentially a wrapper over cotnainer_of. The latter
can never return null even if it's working on inconsistent list since it
will either crash or return some offset in the wrong struct.
Additionally, for the dirty_bgs list the iteration is done under
dirty_bgs_lock which ensures consistency of the list.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index aca906971abe..1b3989c54d7c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4323,11 +4323,6 @@ void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
 		cache = list_first_entry(&cur_trans->dirty_bgs,
 					 struct btrfs_block_group_cache,
 					 dirty_list);
-		if (!cache) {
-			btrfs_err(fs_info, "orphan block group dirty_bgs list");
-			spin_unlock(&cur_trans->dirty_bgs_lock);
-			return;
-		}
 
 		if (!list_empty(&cache->io_list)) {
 			spin_unlock(&cur_trans->dirty_bgs_lock);
@@ -4351,10 +4346,6 @@ void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
 		cache = list_first_entry(&cur_trans->io_bgs,
 					 struct btrfs_block_group_cache,
 					 io_list);
-		if (!cache) {
-			btrfs_err(fs_info, "orphan block group on io_bgs list");
-			return;
-		}
 
 		list_del_init(&cache->io_list);
 		spin_lock(&cache->lock);
-- 
2.7.4

[PATCH 2/2] btrfs: Document consistency of transaction->io_bgs list

[Nikolay Borisov]

The reason why io_bgs can be modified without holding any lock is
non-obvious. Document it and reference that documentation from the
respective call sites

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c     |  4 ++++
 fs/btrfs/extent-tree.c |  7 ++++++-
 fs/btrfs/transaction.h | 15 +++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1b3989c54d7c..b6fc734b0f5c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4342,6 +4342,10 @@ void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
 	}
 	spin_unlock(&cur_trans->dirty_bgs_lock);
 
+
+	/* Refer to the definition of io_bgs member for details why it's safe
+	 * to use it without any locking
+	 */
 	while (!list_empty(&cur_trans->io_bgs)) {
 		cache = list_first_entry(&cur_trans->io_bgs,
 					 struct btrfs_block_group_cache,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index cc08e6af3542..c4a3dfac224a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3742,7 +3742,8 @@ int btrfs_start_dirty_block_groups(struct btrfs_trans_handle *trans,
 
 				/*
 				 * the cache_write_mutex is protecting
-				 * the io_list
+				 * the io_list, also refer to the definition of
+				 * btrfs_transaction::io_bfs for more details
 				 */
 				list_add_tail(&cache->io_list, io);
 			} else {
@@ -3934,6 +3935,10 @@ int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,
 	}
 	spin_unlock(&cur_trans->dirty_bgs_lock);
 
+
+	/* Refer to the definition of io_bgs member for details why it's safe
+	 * to use it without any locking
+	 */
 	while (!list_empty(io)) {
 		cache = list_first_entry(io, struct btrfs_block_group_cache,
 					 io_list);
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 817fd7c9836b..90b80a6bea66 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -69,6 +69,21 @@ struct btrfs_transaction {
 	struct list_head pending_chunks;
 	struct list_head switch_commits;
 	struct list_head dirty_bgs;
+
+	/* There is no explicit lock which protects io_bgs, rather its
+	 * consistency is implied by the fact that all the sites which modify
+	 * it do so under some form of transaction critical section, namely:
+	 *
+	 * * btrfs_start_dirty_block_groups - This function can only ever be
+	 * run by one of the transaction committers. Refer to
+	 * BTRFS_TRANS_DIRTY_BG_RUN usage in btrfs_commit_transaction
+	 *
+	 * * btrfs_write_dirty_blockgroups - this is called by
+	 * commit_cowonly_roots from transaction critical section
+	 * (TRANS_STATE_COMMIT_DOING)
+	 *
+	 * * btrfs_cleanup_dirty_bgs - called on transaction abort
+	 */
 	struct list_head io_bgs;
 	struct list_head dropped_roots;
 
-- 
2.7.4

Re: [PATCH 1/2] btrfs: Remove invalid null checks from btrfs_cleanup_dirty_bgs

[Liu Bo]

On Thu, Feb 08, 2018 at 06:25:17PM +0200, Nikolay Borisov wrote:
> list_first_entry is essentially a wrapper over cotnainer_of. The latter
> can never return null even if it's working on inconsistent list since it
> will either crash or return some offset in the wrong struct.
> Additionally, for the dirty_bgs list the iteration is done under
> dirty_bgs_lock which ensures consistency of the list.
> 

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

-liubo

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/disk-io.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index aca906971abe..1b3989c54d7c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4323,11 +4323,6 @@ void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
>  		cache = list_first_entry(&cur_trans->dirty_bgs,
>  					 struct btrfs_block_group_cache,
>  					 dirty_list);
> -		if (!cache) {
> -			btrfs_err(fs_info, "orphan block group dirty_bgs list");
> -			spin_unlock(&cur_trans->dirty_bgs_lock);
> -			return;
> -		}
>  
>  		if (!list_empty(&cache->io_list)) {
>  			spin_unlock(&cur_trans->dirty_bgs_lock);
> @@ -4351,10 +4346,6 @@ void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
>  		cache = list_first_entry(&cur_trans->io_bgs,
>  					 struct btrfs_block_group_cache,
>  					 io_list);
> -		if (!cache) {
> -			btrfs_err(fs_info, "orphan block group on io_bgs list");
> -			return;
> -		}
>  
>  		list_del_init(&cache->io_list);
>  		spin_lock(&cache->lock);
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] btrfs: Remove invalid null checks from btrfs_cleanup_dirty_bgs

[David Sterba]

On Thu, Feb 08, 2018 at 10:57:11AM -0800, Liu Bo wrote:
> On Thu, Feb 08, 2018 at 06:25:17PM +0200, Nikolay Borisov wrote:
> > list_first_entry is essentially a wrapper over cotnainer_of. The latter
> > can never return null even if it's working on inconsistent list since it
> > will either crash or return some offset in the wrong struct.
> > Additionally, for the dirty_bgs list the iteration is done under
> > dirty_bgs_lock which ensures consistency of the list.
> > 
> 
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Added to next, thanks.