[PATCH 0/7] Corrupt counter improvement

[PATCH 0/7] Corrupt counter improvement

[Nikolay Borisov]

This series aims to make the device corrupt counter be incremented when we
encounter checksum error. This stems from an upstream report at [0] that btrfs
doesn't actually increment the corruption device stats counter. There's no good
reason why this should be the case so here's a patchset rectifying this.

While looking around the code I thought the signature of the functions related
to creating the failrec are somewhat quirky so the first 2 patches fix this.

Patch 3 introduces btrfs_device into btrfs_io_bio so that functions in the
bio completion stack can use it.

Patch 4 removes a redundant check

Next 3 patches wire in increment of the CORRUPT counter in the respective
read end io routines, namely compressed and ordinary reads.

Last patch creates a symlink of the private bdi that btrfs creates on mount
which is used in an xfstest for this series.

[0] https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/

Nikolay Borisov (8):
  btrfs: Make get_state_failrec return failrec directly
  btrfs: Streamline btrfs_get_io_failure_record logic
  btrfs: Record btrfs_device directly btrfs_io_bio
  btrfs: Don't check for btrfs_device::bdev in btrfs_end_bio
  btrfs: Increment device corruption error in case of checksum error
  btrfs: Remove needless ASSERT
  btrfs: Increment corrupt device counter during compressed read
  btrfs: sysfs: Add bdi link to the fsid dir

 fs/btrfs/compression.c    |  10 ++-
 fs/btrfs/ctree.h          |   2 +
 fs/btrfs/extent-io-tree.h |   6 +-
 fs/btrfs/extent_io.c      | 152 +++++++++++++++++++-------------------
 fs/btrfs/inode.c          |   3 +
 fs/btrfs/raid56.c         |   1 +
 fs/btrfs/sysfs.c          |  16 +++-
 fs/btrfs/volumes.c        |  24 +++---
 fs/btrfs/volumes.h        |   2 +-
 9 files changed, 114 insertions(+), 102 deletions(-)

--
2.17.1

[PATCH 1/8] btrfs: Make get_state_failrec return failrec directly

[Nikolay Borisov]

Only failure that get_state_failrec can get is if there is no failurec
for the given address. There is no reason why the function should return
a status code and use a separate parameter for returning the actual
failure rec (if one is found). Simplify it by making the return type
a pointer and return ERR_PTR value in case of errors.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-io-tree.h |  4 ++--
 fs/btrfs/extent_io.c      | 23 ++++++++++++-----------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index b6561455b3c4..62eef5b1dfc6 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -233,8 +233,8 @@ bool btrfs_find_delalloc_range(struct extent_io_tree *tree, u64 *start,
 			       struct extent_state **cached_state);
 
 /* This should be reworked in the future and put elsewhere. */
-int get_state_failrec(struct extent_io_tree *tree, u64 start,
-		      struct io_failure_record **failrec);
+struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
+					    u64 start);
 int set_state_failrec(struct extent_io_tree *tree, u64 start,
 		      struct io_failure_record *failrec);
 void btrfs_free_io_failure_record(struct btrfs_inode *inode, u64 start,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8a7e9da74b87..6f0891ad353b 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2121,12 +2121,12 @@ int set_state_failrec(struct extent_io_tree *tree, u64 start,
 	return ret;
 }
 
-int get_state_failrec(struct extent_io_tree *tree, u64 start,
-		      struct io_failure_record **failrec)
+struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
+					    u64 start)
 {
 	struct rb_node *node;
 	struct extent_state *state;
-	int ret = 0;
+	struct io_failure_record *failrec;
 
 	spin_lock(&tree->lock);
 	/*
@@ -2135,18 +2135,19 @@ int get_state_failrec(struct extent_io_tree *tree, u64 start,
 	 */
 	node = tree_search(tree, start);
 	if (!node) {
-		ret = -ENOENT;
+		failrec = ERR_PTR(-ENOENT);
 		goto out;
 	}
 	state = rb_entry(node, struct extent_state, rb_node);
 	if (state->start != start) {
-		ret = -ENOENT;
+		failrec = ERR_PTR(-ENOENT);
 		goto out;
 	}
-	*failrec = state->failrec;
+
+	failrec = state->failrec;
 out:
 	spin_unlock(&tree->lock);
-	return ret;
+	return failrec;
 }
 
 /*
@@ -2376,8 +2377,8 @@ int clean_io_failure(struct btrfs_fs_info *fs_info,
 	if (!ret)
 		return 0;
 
-	ret = get_state_failrec(failure_tree, start, &failrec);
-	if (ret)
+	failrec = get_state_failrec(failure_tree, start);
+	if (IS_ERR(failrec))
 		return 0;
 
 	BUG_ON(!failrec->this_mirror);
@@ -2461,8 +2462,8 @@ int btrfs_get_io_failure_record(struct inode *inode, u64 start, u64 end,
 	int ret;
 	u64 logical;
 
-	ret = get_state_failrec(failure_tree, start, &failrec);
-	if (ret) {
+	failrec = get_state_failrec(failure_tree, start);
+	if (IS_ERR(failrec)) {
 		failrec = kzalloc(sizeof(*failrec), GFP_NOFS);
 		if (!failrec)
 			return -ENOMEM;
-- 
2.17.1

Re: [PATCH 1/8] btrfs: Make get_state_failrec return failrec directly

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> Only failure that get_state_failrec can get is if there is no failurec
> for the given address. There is no reason why the function should return
> a status code and use a separate parameter for returning the actual
> failure rec (if one is found). Simplify it by making the return type
> a pointer and return ERR_PTR value in case of errors.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/extent-io-tree.h |  4 ++--
>   fs/btrfs/extent_io.c      | 23 ++++++++++++-----------
>   2 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
> index b6561455b3c4..62eef5b1dfc6 100644
> --- a/fs/btrfs/extent-io-tree.h
> +++ b/fs/btrfs/extent-io-tree.h
> @@ -233,8 +233,8 @@ bool btrfs_find_delalloc_range(struct extent_io_tree *tree, u64 *start,
>   			       struct extent_state **cached_state);
>   
>   /* This should be reworked in the future and put elsewhere. */
> -int get_state_failrec(struct extent_io_tree *tree, u64 start,
> -		      struct io_failure_record **failrec);
> +struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
> +					    u64 start);
>   int set_state_failrec(struct extent_io_tree *tree, u64 start,
>   		      struct io_failure_record *failrec);
>   void btrfs_free_io_failure_record(struct btrfs_inode *inode, u64 start,
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 8a7e9da74b87..6f0891ad353b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2121,12 +2121,12 @@ int set_state_failrec(struct extent_io_tree *tree, u64 start,
>   	return ret;
>   }
>   
> -int get_state_failrec(struct extent_io_tree *tree, u64 start,
> -		      struct io_failure_record **failrec)
> +struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
> +					    u64 start)
>   {
>   	struct rb_node *node;
>   	struct extent_state *state;
> -	int ret = 0;
> +	struct io_failure_record *failrec;

Seems we can just do

struct io_failure_record *failrec = ERR_PTR(-ENOENT);

here and avoid the extra stuff below, as we only ever return -ENOENT on failure. 
  Thanks,

Josef

Re: [PATCH 1/8] btrfs: Make get_state_failrec return failrec directly

[David Sterba]

On Thu, Jul 02, 2020 at 09:07:51AM -0400, Josef Bacik wrote:
> On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> > +struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
> > +					    u64 start)
> >   {
> >   	struct rb_node *node;
> >   	struct extent_state *state;
> > -	int ret = 0;
> > +	struct io_failure_record *failrec;
> 
> Seems we can just do
> 
> struct io_failure_record *failrec = ERR_PTR(-ENOENT);
> 
> here and avoid the extra stuff below, as we only ever return -ENOENT on failure. 

I'm not a fan of this pattern, setting the error code just before the
label is IMHO more clear and one does not have to look up the initial
value.

[PATCH 2/8] btrfs: Streamline btrfs_get_io_failure_record logic

[Nikolay Borisov]

Make the function directly return a pointer to a failure record and
adjust callers to handle it. Also refactor the logic inside so that
the case which allocates the failure record for the first time is not
handled in an 'if' arm, saving us a level of indentation. Finally make
the function static as it's not used outside of extent_io.c .

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-io-tree.h |   2 -
 fs/btrfs/extent_io.c      | 131 +++++++++++++++++++-------------------
 2 files changed, 65 insertions(+), 68 deletions(-)

diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index 62eef5b1dfc6..c955b77d8604 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -239,8 +239,6 @@ int set_state_failrec(struct extent_io_tree *tree, u64 start,
 		      struct io_failure_record *failrec);
 void btrfs_free_io_failure_record(struct btrfs_inode *inode, u64 start,
 		u64 end);
-int btrfs_get_io_failure_record(struct inode *inode, u64 start, u64 end,
-				struct io_failure_record **failrec_ret);
 int free_io_failure(struct extent_io_tree *failure_tree,
 		    struct extent_io_tree *io_tree,
 		    struct io_failure_record *rec);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 6f0891ad353b..3a0090ee5e0f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2450,8 +2450,9 @@ void btrfs_free_io_failure_record(struct btrfs_inode *inode, u64 start, u64 end)
 	spin_unlock(&failure_tree->lock);
 }
 
-int btrfs_get_io_failure_record(struct inode *inode, u64 start, u64 end,
-		struct io_failure_record **failrec_ret)
+static
+struct io_failure_record *btrfs_get_io_failure_record(struct inode *inode,
+						      u64 start, u64 end)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct io_failure_record *failrec;
@@ -2463,64 +2464,7 @@ int btrfs_get_io_failure_record(struct inode *inode, u64 start, u64 end,
 	u64 logical;
 
 	failrec = get_state_failrec(failure_tree, start);
-	if (IS_ERR(failrec)) {
-		failrec = kzalloc(sizeof(*failrec), GFP_NOFS);
-		if (!failrec)
-			return -ENOMEM;
-
-		failrec->start = start;
-		failrec->len = end - start + 1;
-		failrec->this_mirror = 0;
-		failrec->bio_flags = 0;
-		failrec->in_validation = 0;
-
-		read_lock(&em_tree->lock);
-		em = lookup_extent_mapping(em_tree, start, failrec->len);
-		if (!em) {
-			read_unlock(&em_tree->lock);
-			kfree(failrec);
-			return -EIO;
-		}
-
-		if (em->start > start || em->start + em->len <= start) {
-			free_extent_map(em);
-			em = NULL;
-		}
-		read_unlock(&em_tree->lock);
-		if (!em) {
-			kfree(failrec);
-			return -EIO;
-		}
-
-		logical = start - em->start;
-		logical = em->block_start + logical;
-		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
-			logical = em->block_start;
-			failrec->bio_flags = EXTENT_BIO_COMPRESSED;
-			extent_set_compress_type(&failrec->bio_flags,
-						 em->compress_type);
-		}
-
-		btrfs_debug(fs_info,
-			"Get IO Failure Record: (new) logical=%llu, start=%llu, len=%llu",
-			logical, start, failrec->len);
-
-		failrec->logical = logical;
-		free_extent_map(em);
-
-		/* set the bits in the private failure tree */
-		ret = set_extent_bits(failure_tree, start, end,
-					EXTENT_LOCKED | EXTENT_DIRTY);
-		if (ret >= 0)
-			ret = set_state_failrec(failure_tree, start, failrec);
-		/* set the bits in the inode's tree */
-		if (ret >= 0)
-			ret = set_extent_bits(tree, start, end, EXTENT_DAMAGED);
-		if (ret < 0) {
-			kfree(failrec);
-			return ret;
-		}
-	} else {
+	if (!IS_ERR(failrec)) {
 		btrfs_debug(fs_info,
 			"Get IO Failure Record: (found) logical=%llu, start=%llu, len=%llu, validation=%d",
 			failrec->logical, failrec->start, failrec->len,
@@ -2530,11 +2474,67 @@ int btrfs_get_io_failure_record(struct inode *inode, u64 start, u64 end,
 		 * (e.g. with a list for failed_mirror) to make
 		 * clean_io_failure() clean all those errors at once.
 		 */
+
+		return failrec;
 	}
 
-	*failrec_ret = failrec;
+	failrec = kzalloc(sizeof(*failrec), GFP_NOFS);
+	if (!failrec)
+		return ERR_PTR(-ENOMEM);
 
-	return 0;
+	failrec->start = start;
+	failrec->len = end - start + 1;
+	failrec->this_mirror = 0;
+	failrec->bio_flags = 0;
+	failrec->in_validation = 0;
+
+	read_lock(&em_tree->lock);
+	em = lookup_extent_mapping(em_tree, start, failrec->len);
+	if (!em) {
+		read_unlock(&em_tree->lock);
+		kfree(failrec);
+		return ERR_PTR(-EIO);
+	}
+
+	if (em->start > start || em->start + em->len <= start) {
+		free_extent_map(em);
+		em = NULL;
+	}
+	read_unlock(&em_tree->lock);
+	if (!em) {
+		kfree(failrec);
+		return ERR_PTR(-EIO);
+	}
+
+	logical = start - em->start;
+	logical = em->block_start + logical;
+	if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
+		logical = em->block_start;
+		failrec->bio_flags = EXTENT_BIO_COMPRESSED;
+		extent_set_compress_type(&failrec->bio_flags,
+					 em->compress_type);
+	}
+
+	btrfs_debug(fs_info,
+		    "Get IO Failure Record: (new) logical=%llu, start=%llu, len=%llu",
+		    logical, start, failrec->len);
+
+	failrec->logical = logical;
+	free_extent_map(em);
+
+	/* set the bits in the private failure tree */
+	ret = set_extent_bits(failure_tree, start, end,
+			      EXTENT_LOCKED | EXTENT_DIRTY);
+	if (ret >= 0) {
+		ret = set_state_failrec(failure_tree, start, failrec);
+		/* set the bits in the inode's tree */
+		ret = set_extent_bits(tree, start, end, EXTENT_DAMAGED);
+	} else if (ret < 0) {
+		kfree(failrec);
+		return ERR_PTR(ret);
+	}
+
+	return failrec;
 }
 
 static bool btrfs_check_repairable(struct inode *inode, bool needs_validation,
@@ -2659,16 +2659,15 @@ blk_status_t btrfs_submit_read_repair(struct inode *inode,
 	struct bio *repair_bio;
 	struct btrfs_io_bio *repair_io_bio;
 	blk_status_t status;
-	int ret;
 
 	btrfs_debug(fs_info,
 		   "repair read error: read error at %llu", start);
 
 	BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
 
-	ret = btrfs_get_io_failure_record(inode, start, end, &failrec);
-	if (ret)
-		return errno_to_blk_status(ret);
+	failrec = btrfs_get_io_failure_record(inode, start, end);
+	if (IS_ERR(failrec))
+		return errno_to_blk_status(PTR_ERR(failrec));
 
 	need_validation = btrfs_io_needs_validation(inode, failed_bio);
 
-- 
2.17.1

Re: [PATCH 2/8] btrfs: Streamline btrfs_get_io_failure_record logic

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> Make the function directly return a pointer to a failure record and
> adjust callers to handle it. Also refactor the logic inside so that
> the case which allocates the failure record for the first time is not
> handled in an 'if' arm, saving us a level of indentation. Finally make
> the function static as it's not used outside of extent_io.c .
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH 3/8] btrfs: Record btrfs_device directly btrfs_io_bio

[Nikolay Borisov]

Instead of recording stripe_index and using that to access correct
btrfs_device from btrfs_bio::stripes record the btrfs_device in
btrfs_io_bio. This will enable endio handlers to increment device
error counters on checksum errors.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/raid56.c  | 1 +
 fs/btrfs/volumes.c | 9 ++-------
 fs/btrfs/volumes.h | 2 +-
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index c870ef70f817..4efd9ed1a30e 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1117,6 +1117,7 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
 
 	/* put a new bio on the list */
 	bio = btrfs_io_bio_alloc(bio_max_len >> PAGE_SHIFT ?: 1);
+	btrfs_io_bio(bio)->dev = stripe->dev;
 	bio->bi_iter.bi_size = 0;
 	bio_set_dev(bio, stripe->dev->bdev);
 	bio->bi_iter.bi_sector = disk_start >> 9;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index aabc6c922e04..9560ac7e9ac9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6261,12 +6261,7 @@ static void btrfs_end_bio(struct bio *bio)
 		atomic_inc(&bbio->error);
 		if (bio->bi_status == BLK_STS_IOERR ||
 		    bio->bi_status == BLK_STS_TARGET) {
-			unsigned int stripe_index =
-				btrfs_io_bio(bio)->stripe_index;
-			struct btrfs_device *dev;
-
-			BUG_ON(stripe_index >= bbio->num_stripes);
-			dev = bbio->stripes[stripe_index].dev;
+			struct btrfs_device *dev = btrfs_io_bio(bio)->dev;
 			if (dev->bdev) {
 				if (bio_op(bio) == REQ_OP_WRITE)
 					btrfs_dev_stat_inc_and_print(dev,
@@ -6319,7 +6314,7 @@ static void submit_stripe_bio(struct btrfs_bio *bbio, struct bio *bio,
 	struct btrfs_fs_info *fs_info = bbio->fs_info;
 
 	bio->bi_private = bbio;
-	btrfs_io_bio(bio)->stripe_index = dev_nr;
+	btrfs_io_bio(bio)->dev = dev;
 	bio->bi_end_io = btrfs_end_bio;
 	bio->bi_iter.bi_sector = physical >> 9;
 	btrfs_debug_in_rcu(fs_info,
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 75af2334b2e3..95aa0cca21e6 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -288,7 +288,7 @@ struct btrfs_fs_devices {
  */
 struct btrfs_io_bio {
 	unsigned int mirror_num;
-	unsigned int stripe_index;
+	struct btrfs_device *dev;
 	u64 logical;
 	u8 *csum;
 	u8 csum_inline[BTRFS_BIO_INLINE_CSUM_SIZE];
-- 
2.17.1

Re: [PATCH 3/8] btrfs: Record btrfs_device directly btrfs_io_bio

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> Instead of recording stripe_index and using that to access correct
> btrfs_device from btrfs_bio::stripes record the btrfs_device in
> btrfs_io_bio. This will enable endio handlers to increment device
> error counters on checksum errors.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 3/8] btrfs: Record btrfs_device directly btrfs_io_bio

[Johannes Thumshirn]

On 02/07/2020 14:40, Nikolay Borisov wrote:
> @@ -6319,7 +6314,7 @@ static void submit_stripe_bio(struct btrfs_bio *bbio, struct bio *bio,
>  	struct btrfs_fs_info *fs_info = bbio->fs_info;
>  
>  	bio->bi_private = bbio;
> -	btrfs_io_bio(bio)->stripe_index = dev_nr;
> +	btrfs_io_bio(bio)->dev = dev;
>  	bio->bi_end_io = btrfs_end_bio;
>  	bio->bi_iter.bi_sector = physical >> 9;
>  	btrfs_debug_in_rcu(fs_info,

Can't we just pass in a btrfs_device here (into submit_stripe_bio() that is) instead of dev_nr?

[PATCH v2] btrfs: Record btrfs_device directly btrfs_io_bio

[Nikolay Borisov]

Instead of recording stripe_index and using that to access correct
btrfs_device from btrfs_bio::stripes record the btrfs_device in
btrfs_io_bio. This will enable endio handlers to increment device
error counters on checksum errors.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2: Pass btrfs_device directly to submit_stripe_bio

 fs/btrfs/raid56.c  |  1 +
 fs/btrfs/volumes.c | 14 ++++----------
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index c870ef70f817..4efd9ed1a30e 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1117,6 +1117,7 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,

 	/* put a new bio on the list */
 	bio = btrfs_io_bio_alloc(bio_max_len >> PAGE_SHIFT ?: 1);
+	btrfs_io_bio(bio)->dev = stripe->dev;
 	bio->bi_iter.bi_size = 0;
 	bio_set_dev(bio, stripe->dev->bdev);
 	bio->bi_iter.bi_sector = disk_start >> 9;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index aabc6c922e04..074256c0cba2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6261,12 +6261,7 @@ static void btrfs_end_bio(struct bio *bio)
 		atomic_inc(&bbio->error);
 		if (bio->bi_status == BLK_STS_IOERR ||
 		    bio->bi_status == BLK_STS_TARGET) {
-			unsigned int stripe_index =
-				btrfs_io_bio(bio)->stripe_index;
-			struct btrfs_device *dev;
-
-			BUG_ON(stripe_index >= bbio->num_stripes);
-			dev = bbio->stripes[stripe_index].dev;
+			struct btrfs_device *dev = btrfs_io_bio(bio)->dev;
 			if (dev->bdev) {
 				if (bio_op(bio) == REQ_OP_WRITE)
 					btrfs_dev_stat_inc_and_print(dev,
@@ -6313,13 +6308,12 @@ static void btrfs_end_bio(struct bio *bio)
 }

 static void submit_stripe_bio(struct btrfs_bio *bbio, struct bio *bio,
-			      u64 physical, int dev_nr)
+			      u64 physical, struct btrfs_device *dev)
 {
-	struct btrfs_device *dev = bbio->stripes[dev_nr].dev;
 	struct btrfs_fs_info *fs_info = bbio->fs_info;

 	bio->bi_private = bbio;
-	btrfs_io_bio(bio)->stripe_index = dev_nr;
+	btrfs_io_bio(bio)->dev = dev;
 	bio->bi_end_io = btrfs_end_bio;
 	bio->bi_iter.bi_sector = physical >> 9;
 	btrfs_debug_in_rcu(fs_info,
@@ -6421,7 +6415,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 			bio = first_bio;

 		submit_stripe_bio(bbio, bio, bbio->stripes[dev_nr].physical,
-				  dev_nr);
+				  dev);
 	}
 	btrfs_bio_counter_dec(fs_info);
 	return BLK_STS_OK;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 75af2334b2e3..95aa0cca21e6 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -288,7 +288,7 @@ struct btrfs_fs_devices {
  */
 struct btrfs_io_bio {
 	unsigned int mirror_num;
-	unsigned int stripe_index;
+	struct btrfs_device *dev;
 	u64 logical;
 	u8 *csum;
 	u8 csum_inline[BTRFS_BIO_INLINE_CSUM_SIZE];
--
2.17.1

Re: [PATCH v2] btrfs: Record btrfs_device directly btrfs_io_bio

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 4/8] btrfs: Don't check for btrfs_device::bdev in btrfs_end_bio

[Nikolay Borisov]

btrfs_map_bio ensures that all submitted bios to devices have valid
btrfs_device::bdev so this check can be removed from btrfs_end_bio. This
check was added in june 2012 597a60fadedf ("Btrfs: don't count I/O
statistic read errors for missing devices")  but then in October of the
same year another commit de1ee92ac3bc ("Btrfs: recheck bio against
block device when we map the bio") started checking for the presence of
btrfs_device::bdev before actually issuing the bio.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9560ac7e9ac9..cb9883c7f8b7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6262,17 +6262,16 @@ static void btrfs_end_bio(struct bio *bio)
 		if (bio->bi_status == BLK_STS_IOERR ||
 		    bio->bi_status == BLK_STS_TARGET) {
 			struct btrfs_device *dev = btrfs_io_bio(bio)->dev;
-			if (dev->bdev) {
-				if (bio_op(bio) == REQ_OP_WRITE)
-					btrfs_dev_stat_inc_and_print(dev,
+			ASSERT(dev->bdev);
+			if (bio_op(bio) == REQ_OP_WRITE)
+				btrfs_dev_stat_inc_and_print(dev,
 						BTRFS_DEV_STAT_WRITE_ERRS);
-				else if (!(bio->bi_opf & REQ_RAHEAD))
-					btrfs_dev_stat_inc_and_print(dev,
+			else if (!(bio->bi_opf & REQ_RAHEAD))
+				btrfs_dev_stat_inc_and_print(dev,
 						BTRFS_DEV_STAT_READ_ERRS);
-				if (bio->bi_opf & REQ_PREFLUSH)
-					btrfs_dev_stat_inc_and_print(dev,
+			if (bio->bi_opf & REQ_PREFLUSH)
+				btrfs_dev_stat_inc_and_print(dev,
 						BTRFS_DEV_STAT_FLUSH_ERRS);
-			}
 		}
 	}
 
-- 
2.17.1

Re: [PATCH 4/8] btrfs: Don't check for btrfs_device::bdev in btrfs_end_bio

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> btrfs_map_bio ensures that all submitted bios to devices have valid
> btrfs_device::bdev so this check can be removed from btrfs_end_bio. This
> check was added in june 2012 597a60fadedf ("Btrfs: don't count I/O
> statistic read errors for missing devices")  but then in October of the
> same year another commit de1ee92ac3bc ("Btrfs: recheck bio against
> block device when we map the bio") started checking for the presence of
> btrfs_device::bdev before actually issuing the bio.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH 5/8] btrfs: Increment device corruption error in case of checksum error

[Nikolay Borisov]

Now that btrfs_io_bio have access to btrfs_device we can safely
increment the device corruption counter on error. There is one notable
exception - repair bios for raid. Since those don't go through the
normal submit_stripe_bio callpath but through raid56_parity_recover thus
repair bios won't have their device set.

Link: https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e7600b0fd9b5..c6824d0ce59d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2822,6 +2822,9 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
 zeroit:
 	btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected,
 				    io_bio->mirror_num);
+	if (io_bio->dev)
+		btrfs_dev_stat_inc_and_print(io_bio->dev,
+					     BTRFS_DEV_STAT_CORRUPTION_ERRS);
 	memset(kaddr + pgoff, 1, len);
 	flush_dcache_page(page);
 	kunmap_atomic(kaddr);
--
2.17.1

Re: [PATCH 5/8] btrfs: Increment device corruption error in case of checksum error

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> Now that btrfs_io_bio have access to btrfs_device we can safely
> increment the device corruption counter on error. There is one notable
> exception - repair bios for raid. Since those don't go through the
> normal submit_stripe_bio callpath but through raid56_parity_recover thus
> repair bios won't have their device set.
> 
> Link: https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/inode.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e7600b0fd9b5..c6824d0ce59d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2822,6 +2822,9 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
>   zeroit:
>   	btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected,
>   				    io_bio->mirror_num);
> +	if (io_bio->dev)
> +		btrfs_dev_stat_inc_and_print(io_bio->dev,
> +					     BTRFS_DEV_STAT_CORRUPTION_ERRS);
>   	memset(kaddr + pgoff, 1, len);
>   	flush_dcache_page(page);
>   	kunmap_atomic(kaddr);
> --
> 2.17.1
> 

I had to go look this up to see if we were double counting, but no, we only do 
BTRFS_DEV_STAT_CORRUPTION_ERRS for data with scrub, which goes through a 
different IO path than the normal reads.  Just in case anybody else is as 
confused as I was

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 5/8] btrfs: Increment device corruption error in case of checksum error

[Johannes Thumshirn]

On 02/07/2020 14:41, Nikolay Borisov wrote:
> Now that btrfs_io_bio have access to btrfs_device we can safely
> increment the device corruption counter on error. There is one notable
> exception - repair bios for raid. Since those don't go through the
> normal submit_stripe_bio callpath but through raid56_parity_recover thus
> repair bios won't have their device set.
> 
> Link: https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e7600b0fd9b5..c6824d0ce59d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2822,6 +2822,9 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
>  zeroit:
>  	btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected,
>  				    io_bio->mirror_num);
> +	if (io_bio->dev)
> +		btrfs_dev_stat_inc_and_print(io_bio->dev,
> +					     BTRFS_DEV_STAT_CORRUPTION_ERRS);
>  	memset(kaddr + pgoff, 1, len);
>  	flush_dcache_page(page);
>  	kunmap_atomic(kaddr);

Any chance you could do a follow up merging that weird zeroit label 
into the memset() block?

It kind of disturbs the reading flow of that function and in fact it 
doesn't even zero the data

Re: [PATCH 5/8] btrfs: Increment device corruption error in case of checksum error

[Nikolay Borisov]

On 2.07.20 г. 16:21 ч., Johannes Thumshirn wrote:
> On 02/07/2020 14:41, Nikolay Borisov wrote:
>> Now that btrfs_io_bio have access to btrfs_device we can safely
>> increment the device corruption counter on error. There is one notable
>> exception - repair bios for raid. Since those don't go through the
>> normal submit_stripe_bio callpath but through raid56_parity_recover thus
>> repair bios won't have their device set.
>>
>> Link: https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/inode.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index e7600b0fd9b5..c6824d0ce59d 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -2822,6 +2822,9 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
>>  zeroit:
>>  	btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected,
>>  				    io_bio->mirror_num);
>> +	if (io_bio->dev)
>> +		btrfs_dev_stat_inc_and_print(io_bio->dev,
>> +					     BTRFS_DEV_STAT_CORRUPTION_ERRS);
>>  	memset(kaddr + pgoff, 1, len);
>>  	flush_dcache_page(page);
>>  	kunmap_atomic(kaddr);
> 
> Any chance you could do a follow up merging that weird zeroit label 
> into the memset() block?
> 
> It kind of disturbs the reading flow of that function and in fact it 
> doesn't even zero the data
> 

Makes sense, it doesn't even look that bad at all in terms of line length.

[PATCH 6/8] btrfs: Remove needless ASSERT

[Nikolay Borisov]

compressed_bio::orig_bio is always set in btrfs_submit_compressed_read
before any bio submission is performed. Since that function is always
called with a valid bio it renders the ASSERT unnecessary.

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

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index c2d5ca583dbf..db80c3fa6c08 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -244,7 +244,6 @@ static void end_compressed_bio_read(struct bio *bio)
 	 * Record the correct mirror_num in cb->orig_bio so that
 	 * read-repair can work properly.
 	 */
-	ASSERT(btrfs_io_bio(cb->orig_bio));
 	btrfs_io_bio(cb->orig_bio)->mirror_num = mirror;
 	cb->mirror_num = mirror;
 
-- 
2.17.1

Re: [PATCH 6/8] btrfs: Remove needless ASSERT

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> compressed_bio::orig_bio is always set in btrfs_submit_compressed_read
> before any bio submission is performed. Since that function is always
> called with a valid bio it renders the ASSERT unnecessary.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/compression.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index c2d5ca583dbf..db80c3fa6c08 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -244,7 +244,6 @@ static void end_compressed_bio_read(struct bio *bio)
>   	 * Record the correct mirror_num in cb->orig_bio so that
>   	 * read-repair can work properly.
>   	 */
> -	ASSERT(btrfs_io_bio(cb->orig_bio));
>   	btrfs_io_bio(cb->orig_bio)->mirror_num = mirror;
>   	cb->mirror_num = mirror;
>   
> 

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 6/8] btrfs: Remove needless ASSERT

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 7/8] btrfs: Increment corrupt device counter during compressed read

[Nikolay Borisov]

If a compressed read fails due to csum error only a line is printed to
dmesg, device corrupt counter is not modified.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/compression.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index db80c3fa6c08..2f30bf4127f8 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -172,8 +172,7 @@ static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
 		(DIV_ROUND_UP(disk_size, fs_info->sectorsize)) * csum_size;
 }
 
-static int check_compressed_csum(struct btrfs_inode *inode,
-				 struct compressed_bio *cb,
+static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
 				 u64 disk_start)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
@@ -184,6 +183,7 @@ static int check_compressed_csum(struct btrfs_inode *inode,
 	unsigned long i;
 	char *kaddr;
 	u8 csum[BTRFS_CSUM_SIZE];
+	struct compressed_bio *cb = bio->bi_private;
 	u8 *cb_sum = cb->sums;
 
 	if (inode->flags & BTRFS_INODE_NODATASUM)
@@ -201,6 +201,9 @@ static int check_compressed_csum(struct btrfs_inode *inode,
 		if (memcmp(&csum, cb_sum, csum_size)) {
 			btrfs_print_data_csum_error(inode, disk_start,
 					csum, cb_sum, cb->mirror_num);
+			if (btrfs_io_bio(bio)->dev)
+				btrfs_dev_stat_inc_and_print(btrfs_io_bio(bio)->dev,
+					BTRFS_DEV_STAT_CORRUPTION_ERRS);
 			ret = -EIO;
 			goto fail;
 		}
@@ -255,7 +258,7 @@ static void end_compressed_bio_read(struct bio *bio)
 		goto csum_failed;
 
 	inode = cb->inode;
-	ret = check_compressed_csum(BTRFS_I(inode), cb,
+	ret = check_compressed_csum(BTRFS_I(inode), bio,
 				    (u64)bio->bi_iter.bi_sector << 9);
 	if (ret)
 		goto csum_failed;
-- 
2.17.1

Re: [PATCH 7/8] btrfs: Increment corrupt device counter during compressed read

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> If a compressed read fails due to csum error only a line is printed to
> dmesg, device corrupt counter is not modified.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 7/8] btrfs: Increment corrupt device counter during compressed read

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 8/8] btrfs: sysfs: Add bdi link to the fsid dir

[Nikolay Borisov]

Since BTRFS uses a private bdi it makes sense to create a link to this
bdi under /sys/fs/btrfs/<UUID>/bdi. This allows size of read ahead to
be controlled. Without this patch it's not possible to uniquely identify
which bdi pertains to which btrfs filesystem in the fase of multiple
btrfs filesystem.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h |  2 ++
 fs/btrfs/sysfs.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4dd478b4fe3a..eb61f89e9e85 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -928,6 +928,8 @@ struct btrfs_fs_info {
 	u32 sectorsize;
 	u32 stripesize;
 
+	bool bdi_link_created;
+
 	/* Block groups and devices containing active swapfiles. */
 	spinlock_t swapfile_pins_lock;
 	struct rb_root swapfile_pins;
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5885abe57c3e..e167ec584627 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -937,8 +937,13 @@ void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs)
 
 void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 {
+	struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj;
+
 	btrfs_reset_fs_info_ptr(fs_info);
 
+	if (fs_info->bdi_link_created)
+		sysfs_remove_link(fsid_kobj, "bdi");
+
 	if (fs_info->space_info_kobj) {
 		sysfs_remove_files(fs_info->space_info_kobj, allocation_attrs);
 		kobject_del(fs_info->space_info_kobj);
@@ -958,8 +963,8 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 	}
 #endif
 	addrm_unknown_feature_attrs(fs_info, false);
-	sysfs_remove_group(&fs_info->fs_devices->fsid_kobj, &btrfs_feature_attr_group);
-	sysfs_remove_files(&fs_info->fs_devices->fsid_kobj, btrfs_attrs);
+	sysfs_remove_group(fsid_kobj, &btrfs_feature_attr_group);
+	sysfs_remove_files(fsid_kobj, btrfs_attrs);
 	btrfs_sysfs_remove_devices_dir(fs_info->fs_devices, NULL);
 }
 
@@ -1410,6 +1415,13 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
 	if (error)
 		goto failure;
 
+	error = sysfs_create_link(fsid_kobj, &fs_info->sb->s_bdi->dev->kobj,
+				  "bdi");
+	if (error)
+		goto failure;
+
+	fs_info->bdi_link_created = true;
+
 #ifdef CONFIG_BTRFS_DEBUG
 	fs_info->debug_kobj = kobject_create_and_add("debug", fsid_kobj);
 	if (!fs_info->debug_kobj) {
-- 
2.17.1

Re: [PATCH 8/8] btrfs: sysfs: Add bdi link to the fsid dir

[Josef Bacik]

On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> Since BTRFS uses a private bdi it makes sense to create a link to this
> bdi under /sys/fs/btrfs/<UUID>/bdi. This allows size of read ahead to
> be controlled. Without this patch it's not possible to uniquely identify
> which bdi pertains to which btrfs filesystem in the fase of multiple
> btrfs filesystem.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Was confused why we needed to make sure the link existed before removing it, 
since other things sysfs is smart enough to figure out.  Apparently it has a 
WARN_ON() if the parent isn't initialized, so the check is necessary, albeit 
annoying.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 8/8] btrfs: sysfs: Add bdi link to the fsid dir

[David Sterba]

On Thu, Jul 02, 2020 at 09:25:30AM -0400, Josef Bacik wrote:
> On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> > Since BTRFS uses a private bdi it makes sense to create a link to this
> > bdi under /sys/fs/btrfs/<UUID>/bdi. This allows size of read ahead to
> > be controlled. Without this patch it's not possible to uniquely identify
> > which bdi pertains to which btrfs filesystem in the fase of multiple
> > btrfs filesystem.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Was confused why we needed to make sure the link existed before removing it, 
> since other things sysfs is smart enough to figure out.  Apparently it has a 
> WARN_ON() if the parent isn't initialized, so the check is necessary, albeit 
> annoying.

There must be a better way, this is just too weird. We can check if
objects have been initialized by peeking to kobject::state_initialized
and we use that already for fsid_kobj in __btrfs_sysfs_remove_fsid or
btrfs_sysfs_feature_update.

Re: [PATCH 8/8] btrfs: sysfs: Add bdi link to the fsid dir

[Nikolay Borisov]

On 2.07.20 г. 16:36 ч., David Sterba wrote:
> On Thu, Jul 02, 2020 at 09:25:30AM -0400, Josef Bacik wrote:
>> On 7/2/20 8:23 AM, Nikolay Borisov wrote:
>>> Since BTRFS uses a private bdi it makes sense to create a link to this
>>> bdi under /sys/fs/btrfs/<UUID>/bdi. This allows size of read ahead to
>>> be controlled. Without this patch it's not possible to uniquely identify
>>> which bdi pertains to which btrfs filesystem in the fase of multiple
>>> btrfs filesystem.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>
>> Was confused why we needed to make sure the link existed before removing it, 
>> since other things sysfs is smart enough to figure out.  Apparently it has a 
>> WARN_ON() if the parent isn't initialized, so the check is necessary, albeit 
>> annoying.
> 
> There must be a better way, this is just too weird. We can check if
> objects have been initialized by peeking to kobject::state_initialized
> and we use that already for fsid_kobj in __btrfs_sysfs_remove_fsid or
> btrfs_sysfs_feature_update.
> 

Awesome, will use this for v2.

[PATCH v2] btrfs: sysfs: Add bdi link to the fsid dir

[Nikolay Borisov]

Since BTRFS uses a private bdi it makes sense to create a link to this
bdi under /sys/fs/btrfs/<UUID>/bdi. This allows size of read ahead to
be controlled. Without this patch it's not possible to uniquely identify
which bdi pertains to which btrfs filesystem in the fase of multiple
btrfs filesystem.

It's fine to simply call sysfs_remove_link without checking if the
link indeed has been created. The call path
sysfs_remove_link
 kernfs_remove_by_name
  kernfs_remove_by_name_ns

Will simply return -ENOENT in case it doesn't exist.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2: Remove variable signalling whether the 'bdi' symlinkn is created. Turns out
it's not really neede for proper error handling.

 fs/btrfs/sysfs.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5885abe57c3e..e766bfecd874 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -937,8 +937,12 @@ void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs)

 void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 {
+	struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj;
+
 	btrfs_reset_fs_info_ptr(fs_info);

+	sysfs_remove_link(fsid_kobj, "bdi");
+
 	if (fs_info->space_info_kobj) {
 		sysfs_remove_files(fs_info->space_info_kobj, allocation_attrs);
 		kobject_del(fs_info->space_info_kobj);
@@ -958,8 +962,8 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 	}
 #endif
 	addrm_unknown_feature_attrs(fs_info, false);
-	sysfs_remove_group(&fs_info->fs_devices->fsid_kobj, &btrfs_feature_attr_group);
-	sysfs_remove_files(&fs_info->fs_devices->fsid_kobj, btrfs_attrs);
+	sysfs_remove_group(fsid_kobj, &btrfs_feature_attr_group);
+	sysfs_remove_files(fsid_kobj, btrfs_attrs);
 	btrfs_sysfs_remove_devices_dir(fs_info->fs_devices, NULL);
 }

@@ -1439,6 +1443,11 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
 	if (error)
 		goto failure;

+	error = sysfs_create_link(fsid_kobj, &fs_info->sb->s_bdi->dev->kobj,
+				  "bdi");
+	if (error)
+		goto failure;
+
 	fs_info->space_info_kobj = kobject_create_and_add("allocation",
 						  fsid_kobj);
 	if (!fs_info->space_info_kobj) {
--
2.17.1

Re: [PATCH v2] btrfs: sysfs: Add bdi link to the fsid dir

[Anand Jain]

Adds little cleanup also.

Looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH 0/7] Corrupt counter improvement

[David Sterba]

On Thu, Jul 02, 2020 at 03:23:27PM +0300, Nikolay Borisov wrote:
> This series aims to make the device corrupt counter be incremented when we
> encounter checksum error. This stems from an upstream report at [0] that btrfs
> doesn't actually increment the corruption device stats counter. There's no good
> reason why this should be the case so here's a patchset rectifying this.

Yeah I think this was forgotten at the time the dev-stats were merged.

> While looking around the code I thought the signature of the functions related
> to creating the failrec are somewhat quirky so the first 2 patches fix this.
> 
> Patch 3 introduces btrfs_device into btrfs_io_bio so that functions in the
> bio completion stack can use it.
> 
> Patch 4 removes a redundant check
> 
> Next 3 patches wire in increment of the CORRUPT counter in the respective
> read end io routines, namely compressed and ordinary reads.
> 
> Last patch creates a symlink of the private bdi that btrfs creates on mount
> which is used in an xfstest for this series.
> 
> [0] https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/
> 
> Nikolay Borisov (8):
>   btrfs: Make get_state_failrec return failrec directly
>   btrfs: Streamline btrfs_get_io_failure_record logic
>   btrfs: Record btrfs_device directly btrfs_io_bio
>   btrfs: Don't check for btrfs_device::bdev in btrfs_end_bio
>   btrfs: Increment device corruption error in case of checksum error
>   btrfs: Remove needless ASSERT
>   btrfs: Increment corrupt device counter during compressed read
>   btrfs: sysfs: Add bdi link to the fsid dir

This is pretty straightforward, thanks. I did some smallish changes like
renaming the btrfs_io_bio::dev to device.

Updating the existing counter is in line with scrub so we don't have to
change the on-disk stats.

Patchset is now in misc-next, I don't see any reason to keep it in a
topic branch.