Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, Nov 04 2013 at 12:49pm -0500, Mikulas Patocka wrote: > > > On Mon, 4 Nov 2013, Mike Snitzer wrote: > > > On Mon, Nov 04 2013 at 10:25am -0500, > > Mikulas Patocka wrote: > > > > > > > > > > > On Mon, 4 Nov 2013, Mike Snitzer wrote: > > > > > > > On Mon, Nov 04 2013 at 10:06am -0500, > > > > Mikulas Patocka 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
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 > > 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). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, 4 Nov 2013, Mike Snitzer wrote: > On Mon, Nov 04 2013 at 10:25am -0500, > Mikulas Patocka wrote: > > > > > > > On Mon, 4 Nov 2013, Mike Snitzer wrote: > > > > > On Mon, Nov 04 2013 at 10:06am -0500, > > > Mikulas Patocka 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, Nov 04 2013 at 10:25am -0500, Mikulas Patocka wrote: > > > On Mon, 4 Nov 2013, Mike Snitzer wrote: > > > On Mon, Nov 04 2013 at 10:06am -0500, > > Mikulas Patocka 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, 4 Nov 2013, Mike Snitzer wrote: > On Mon, Nov 04 2013 at 10:06am -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. > > > > > > From: Mikulas Patocka > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, Nov 04 2013 at 10:06am -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. > > > From: Mikulas Patocka > > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
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
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
---
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(-)
Index: linux-block/drivers/md/dm-cache-target.c
===
--- linux-block.orig/drivers/md/dm-cache-target.c 2013-11-01
17:21:11.0 +0100
+++ linux-block/drivers/md/dm-cache-target.c2013-11-04 13:59:13.0
+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(>bi_remaining);
+ atomic_set(>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.0
+0100
+++ linux-block/drivers/md/dm-snap.c2013-11-04 13:59:56.0 +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(_bio->bi_remaining);
+ atomic_set(_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.0
+0100
+++ linux-block/drivers/md/dm-thin.c2013-11-04 14:00:19.0 +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(>bio->bi_remaining);
+ atomic_set(>bio->bi_remaining, 1);
}
cell_error(m->tc->pool, m->cell);
list_del(>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(>bi_remaining);
+ atomic_set(>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.0
+0100
+++ linux-block/drivers/md/dm-verity.c 2013-11-04 13:59:28.0 +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(>bi_remaining);
+ atomic_set(>bi_remaining, 1);
bio_endio(bio, error);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, 4 Nov 2013, Mike Snitzer wrote: On Mon, Nov 04 2013 at 10:25am -0500, Mikulas Patocka mpato...@redhat.com wrote: On Mon, 4 Nov 2013, Mike Snitzer wrote: On Mon, Nov 04 2013 at 10:06am -0500, Mikulas Patocka mpato...@redhat.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
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 mpato...@redhat.com 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). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, Nov 04 2013 at 12:49pm -0500, Mikulas Patocka mpato...@redhat.com wrote: On Mon, 4 Nov 2013, Mike Snitzer wrote: On Mon, Nov 04 2013 at 10:25am -0500, Mikulas Patocka mpato...@redhat.com wrote: On Mon, 4 Nov 2013, Mike Snitzer wrote: On Mon, Nov 04 2013 at 10:06am -0500, Mikulas Patocka mpato...@redhat.com 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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
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 mpato...@redhat.com
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 mpato...@redhat.com
---
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(-)
Index: linux-block/drivers/md/dm-cache-target.c
===
--- linux-block.orig/drivers/md/dm-cache-target.c 2013-11-01
17:21:11.0 +0100
+++ linux-block/drivers/md/dm-cache-target.c2013-11-04 13:59:13.0
+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.0
+0100
+++ linux-block/drivers/md/dm-snap.c2013-11-04 13:59:56.0 +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.0
+0100
+++ linux-block/drivers/md/dm-thin.c2013-11-04 14:00:19.0 +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.0
+0100
+++ linux-block/drivers/md/dm-verity.c 2013-11-04 13:59:28.0 +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);
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, Nov 04 2013 at 10:06am -0500, Mikulas Patocka mpato...@redhat.com 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 mpato...@redhat.com 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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, 4 Nov 2013, Mike Snitzer wrote: On Mon, Nov 04 2013 at 10:06am -0500, Mikulas Patocka mpato...@redhat.com 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 mpato...@redhat.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
On Mon, Nov 04 2013 at 10:25am -0500, Mikulas Patocka mpato...@redhat.com wrote: On Mon, 4 Nov 2013, Mike Snitzer wrote: On Mon, Nov 04 2013 at 10:06am -0500, Mikulas Patocka mpato...@redhat.com 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for-next] dm: fix missing bi_remaining accounting
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
