[for-next] dm: fix missing bi_remaining accounting

Commit Message

Mikulas Patocka Nov. 4, 2013, 3:06 p.m. UTC

On Fri, 1 Nov 2013, Jens Axboe wrote:

> On 11/01/2013 07:59 AM, Mike Snitzer wrote:
> > Add the missing bi_remaining increment, required by the block layer's
> > new bio-chaining code, to both the verity and old snapshot DM targets.
> > 
> > Otherwise users will hit the bi_remaining <= 0 BUG_ON in bio_endio().
> 
> Thanks Mike, added to the mix.
> 
> -- 
> Jens Axboe

Hi

This improves a little bit on the previous patch, by replacing costly 
atomic_inc with cheap atomic_set.


From: Mikulas Patocka <[e-mail]>

dm: change atomic_inc to atomic_set(1)

There are places in dm where we save bi_endio and bi_private, set them to
target's routine, submit the bio, from the target's bi_endio routine we
restore bi_endio and bi_private and end the bio with bi_endio.

This causes underflow of bi_remaining, so we must restore bi_remaining
before ending the bio from the target bi_endio routine.

The code uses atomic_inc for restoration of bi_remaining. This patch
changes it to atomic_set(1) to avoid an interlocked instruction. In the
target's bi_endio routine we are sure that bi_remaining is zero
(otherwise, the bi_endio routine wouldn't be called) and there are no
concurrent users of the bio, so we can replace atomic_inc with
atomic_set(1).

Signed-off-by: Mikulas Patocka <[e-mail]>

---
 drivers/md/dm-cache-target.c |    2 +-
 drivers/md/dm-snap.c         |    2 +-
 drivers/md/dm-thin.c         |    4 ++--
 drivers/md/dm-verity.c       |    2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Mike Snitzer Nov. 4, 2013, 3:20 p.m. UTC | #1

On Mon, Nov 04 2013 at 10:06am -0500,
Mikulas Patocka <[e-mail]> wrote:

> 
> 
> On Fri, 1 Nov 2013, Jens Axboe wrote:
> 
> > On 11/01/2013 07:59 AM, Mike Snitzer wrote:
> > > Add the missing bi_remaining increment, required by the block layer's
> > > new bio-chaining code, to both the verity and old snapshot DM targets.
> > > 
> > > Otherwise users will hit the bi_remaining <= 0 BUG_ON in bio_endio().
> > 
> > Thanks Mike, added to the mix.
> > 
> > -- 
> > Jens Axboe
> 
> Hi
> 
> This improves a little bit on the previous patch, by replacing costly 
> atomic_inc with cheap atomic_set.
> 
> 
> From: Mikulas Patocka <[e-mail]>
> 
> dm: change atomic_inc to atomic_set(1)
> 
> There are places in dm where we save bi_endio and bi_private, set them to
> target's routine, submit the bio, from the target's bi_endio routine we
> restore bi_endio and bi_private and end the bio with bi_endio.
> 
> This causes underflow of bi_remaining, so we must restore bi_remaining
> before ending the bio from the target bi_endio routine.
> 
> The code uses atomic_inc for restoration of bi_remaining. This patch
> changes it to atomic_set(1) to avoid an interlocked instruction. In the
> target's bi_endio routine we are sure that bi_remaining is zero
> (otherwise, the bi_endio routine wouldn't be called) and there are no
> concurrent users of the bio, so we can replace atomic_inc with
> atomic_set(1).

This isn't DM-specific.  Shouldn't the other places in the tree that use
atomic_inc on bi_remaining should really be converted at the same time?

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Mikulas Patocka Nov. 4, 2013, 3:25 p.m. UTC | #2

On Mon, 4 Nov 2013, Mike Snitzer wrote:

> On Mon, Nov 04 2013 at 10:06am -0500,
> Mikulas Patocka <[e-mail]> wrote:
> 
> > 
> > 
> > On Fri, 1 Nov 2013, Jens Axboe wrote:
> > 
> > > On 11/01/2013 07:59 AM, Mike Snitzer wrote:
> > > > Add the missing bi_remaining increment, required by the block layer's
> > > > new bio-chaining code, to both the verity and old snapshot DM targets.
> > > > 
> > > > Otherwise users will hit the bi_remaining <= 0 BUG_ON in bio_endio().
> > > 
> > > Thanks Mike, added to the mix.
> > > 
> > > -- 
> > > Jens Axboe
> > 
> > Hi
> > 
> > This improves a little bit on the previous patch, by replacing costly 
> > atomic_inc with cheap atomic_set.
> > 
> > 
> > From: Mikulas Patocka <[e-mail]>
> > 
> > dm: change atomic_inc to atomic_set(1)
> > 
> > There are places in dm where we save bi_endio and bi_private, set them to
> > target's routine, submit the bio, from the target's bi_endio routine we
> > restore bi_endio and bi_private and end the bio with bi_endio.
> > 
> > This causes underflow of bi_remaining, so we must restore bi_remaining
> > before ending the bio from the target bi_endio routine.
> > 
> > The code uses atomic_inc for restoration of bi_remaining. This patch
> > changes it to atomic_set(1) to avoid an interlocked instruction. In the
> > target's bi_endio routine we are sure that bi_remaining is zero
> > (otherwise, the bi_endio routine wouldn't be called) and there are no
> > concurrent users of the bio, so we can replace atomic_inc with
> > atomic_set(1).
> 
> This isn't DM-specific.  Shouldn't the other places in the tree that use
> atomic_inc on bi_remaining should really be converted at the same time?

There is no 'atomic_inc.*bi_remaining' in other drivers.

It is just in fs/bio.c in bio_chain and bio_endio_nodec where it's 
probably needed.

Mikulas

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Mike Snitzer Nov. 4, 2013, 4:04 p.m. UTC | #3

On Mon, Nov 04 2013 at 10:25am -0500,
Mikulas Patocka <[e-mail]> wrote:

> 
> 
> On Mon, 4 Nov 2013, Mike Snitzer wrote:
> 
> > On Mon, Nov 04 2013 at 10:06am -0500,
> > Mikulas Patocka <[e-mail]> wrote:
> > > 
> > > The code uses atomic_inc for restoration of bi_remaining. This patch
> > > changes it to atomic_set(1) to avoid an interlocked instruction. In the
> > > target's bi_endio routine we are sure that bi_remaining is zero
> > > (otherwise, the bi_endio routine wouldn't be called) and there are no
> > > concurrent users of the bio, so we can replace atomic_inc with
> > > atomic_set(1).
> > 
> > This isn't DM-specific.  Shouldn't the other places in the tree that use
> > atomic_inc on bi_remaining should really be converted at the same time?
> 
> There is no 'atomic_inc.*bi_remaining' in other drivers.

Wrong.  I know btrfs has at least one.  As does bcache afaik.

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Mikulas Patocka Nov. 4, 2013, 5:49 p.m. UTC | #4

On Mon, 4 Nov 2013, Mike Snitzer wrote:

> On Mon, Nov 04 2013 at 10:25am -0500,
> Mikulas Patocka <[e-mail]> wrote:
> 
> > 
> > 
> > On Mon, 4 Nov 2013, Mike Snitzer wrote:
> > 
> > > On Mon, Nov 04 2013 at 10:06am -0500,
> > > Mikulas Patocka <[e-mail]> wrote:
> > > > 
> > > > The code uses atomic_inc for restoration of bi_remaining. This patch
> > > > changes it to atomic_set(1) to avoid an interlocked instruction. In the
> > > > target's bi_endio routine we are sure that bi_remaining is zero
> > > > (otherwise, the bi_endio routine wouldn't be called) and there are no
> > > > concurrent users of the bio, so we can replace atomic_inc with
> > > > atomic_set(1).
> > > 
> > > This isn't DM-specific.  Shouldn't the other places in the tree that use
> > > atomic_inc on bi_remaining should really be converted at the same time?
> > 
> > There is no 'atomic_inc.*bi_remaining' in other drivers.
> 
> Wrong.  I know btrfs has at least one.  As does bcache afaik.

grep -r 'atomic_inc.*bi_remaining' * yilds no hits in btrfs or bcache (on 
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git , 
branch remotes/origin/for-next). It only finds 
drivers/md/dm-cache-target.c, drivers/md/dm-verity.c, 
drivers/md/dm-snap.c, drivers/md/dm-thin.c. Maybe in other git trees there 
are more cases of this.

Mikulas

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Kent Overstreet Nov. 4, 2013, 8:12 p.m. UTC | #5

On Mon, Nov 04, 2013 at 10:06:00AM -0500, Mikulas Patocka wrote:
> 
> 
> On Fri, 1 Nov 2013, Jens Axboe wrote:
> 
> > On 11/01/2013 07:59 AM, Mike Snitzer wrote:
> > > Add the missing bi_remaining increment, required by the block layer's
> > > new bio-chaining code, to both the verity and old snapshot DM targets.
> > > 
> > > Otherwise users will hit the bi_remaining <= 0 BUG_ON in bio_endio().
> > 
> > Thanks Mike, added to the mix.
> > 
> > -- 
> > Jens Axboe
> 
> Hi
> 
> This improves a little bit on the previous patch, by replacing costly 
> atomic_inc with cheap atomic_set.

IMO, this is a bad idea; the behaviour with this patch does _not_ match the
naming of bio_endio_nodec(), and the performance difference should be well in
the noise anyways because we're touching a cacheline we already have in cache
and won't be contended.

The fact that it's currently safe is accidental, I could see this easily
tripping people up and being a pain in the ass to debug in the future.

> 
> 
> From: Mikulas Patocka <[e-mail]>
> 
> dm: change atomic_inc to atomic_set(1)
> 
> There are places in dm where we save bi_endio and bi_private, set them to
> target's routine, submit the bio, from the target's bi_endio routine we
> restore bi_endio and bi_private and end the bio with bi_endio.
> 
> This causes underflow of bi_remaining, so we must restore bi_remaining
> before ending the bio from the target bi_endio routine.
> 
> The code uses atomic_inc for restoration of bi_remaining. This patch
> changes it to atomic_set(1) to avoid an interlocked instruction. In the
> target's bi_endio routine we are sure that bi_remaining is zero
> (otherwise, the bi_endio routine wouldn't be called) and there are no
> concurrent users of the bio, so we can replace atomic_inc with
> atomic_set(1).

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Mike Snitzer Nov. 5, 2013, 12:56 a.m. UTC | #6

On Mon, Nov 04 2013 at 12:49pm -0500,
Mikulas Patocka <[e-mail]> wrote:

> 
> 
> On Mon, 4 Nov 2013, Mike Snitzer wrote:
> 
> > On Mon, Nov 04 2013 at 10:25am -0500,
> > Mikulas Patocka <[e-mail]> wrote:
> > 
> > > 
> > > 
> > > On Mon, 4 Nov 2013, Mike Snitzer wrote:
> > > 
> > > > On Mon, Nov 04 2013 at 10:06am -0500,
> > > > Mikulas Patocka <[e-mail]> wrote:
> > > > > 
> > > > > The code uses atomic_inc for restoration of bi_remaining. This patch
> > > > > changes it to atomic_set(1) to avoid an interlocked instruction. In the
> > > > > target's bi_endio routine we are sure that bi_remaining is zero
> > > > > (otherwise, the bi_endio routine wouldn't be called) and there are no
> > > > > concurrent users of the bio, so we can replace atomic_inc with
> > > > > atomic_set(1).
> > > > 
> > > > This isn't DM-specific.  Shouldn't the other places in the tree that use
> > > > atomic_inc on bi_remaining should really be converted at the same time?
> > > 
> > > There is no 'atomic_inc.*bi_remaining' in other drivers.
> > 
> > Wrong.  I know btrfs has at least one.  As does bcache afaik.
> 
> grep -r 'atomic_inc.*bi_remaining' * yilds no hits in btrfs or bcache (on 
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git , 
> branch remotes/origin/for-next). It only finds 
> drivers/md/dm-cache-target.c, drivers/md/dm-verity.c, 
> drivers/md/dm-snap.c, drivers/md/dm-thin.c. Maybe in other git trees there 
> are more cases of this.

Here is the btrfs one I was thinking of from Chris:
https://patchwork.kernel.org/patch/3123121/

Should probably make its way into linux-block.git, Jens?

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

Index: linux-block/drivers/md/dm-cache-target.c
===================================================================
--- linux-block.orig/drivers/md/dm-cache-target.c	2013-11-01 17:21:11.000000000 +0100
+++ linux-block/drivers/md/dm-cache-target.c	2013-11-04 13:59:13.000000000 +0100
@@ -670,7 +670,7 @@  static void writethrough_endio(struct bi
 	 * Must bump bi_remaining to allow bio to complete with
 	 * restored bi_end_io.
 	 */
-	atomic_inc(&bio->bi_remaining);
+	atomic_set(&bio->bi_remaining, 1);
 
 	if (err) {
 		bio_endio(bio, err);
Index: linux-block/drivers/md/dm-snap.c
===================================================================
--- linux-block.orig/drivers/md/dm-snap.c	2013-11-01 17:21:11.000000000 +0100
+++ linux-block/drivers/md/dm-snap.c	2013-11-04 13:59:56.000000000 +0100
@@ -1415,7 +1415,7 @@  out:
 	if (full_bio) {
 		full_bio->bi_end_io = pe->full_bio_end_io;
 		full_bio->bi_private = pe->full_bio_private;
-		atomic_inc(&full_bio->bi_remaining);
+		atomic_set(&full_bio->bi_remaining, 1);
 	}
 	free_pending_exception(pe);
 
Index: linux-block/drivers/md/dm-thin.c
===================================================================
--- linux-block.orig/drivers/md/dm-thin.c	2013-11-01 17:21:11.000000000 +0100
+++ linux-block/drivers/md/dm-thin.c	2013-11-04 14:00:19.000000000 +0100
@@ -613,7 +613,7 @@  static void process_prepared_mapping_fai
 {
 	if (m->bio) {
 		m->bio->bi_end_io = m->saved_bi_end_io;
-		atomic_inc(&m->bio->bi_remaining);
+		atomic_set(&m->bio->bi_remaining, 1);
 	}
 	cell_error(m->tc->pool, m->cell);
 	list_del(&m->list);
@@ -630,7 +630,7 @@  static void process_prepared_mapping(str
 	bio = m->bio;
 	if (bio) {
 		bio->bi_end_io = m->saved_bi_end_io;
-		atomic_inc(&bio->bi_remaining);
+		atomic_set(&bio->bi_remaining, 1);
 	}
 
 	if (m->err) {
Index: linux-block/drivers/md/dm-verity.c
===================================================================
--- linux-block.orig/drivers/md/dm-verity.c	2013-11-01 17:21:11.000000000 +0100
+++ linux-block/drivers/md/dm-verity.c	2013-11-04 13:59:28.000000000 +0100
@@ -384,7 +384,7 @@  static void verity_finish_io(struct dm_v
 
 	bio->bi_end_io = io->orig_bi_end_io;
 	bio->bi_private = io->orig_bi_private;
-	atomic_inc(&bio->bi_remaining);
+	atomic_set(&bio->bi_remaining, 1);
 
 	bio_endio(bio, error);
 }