* [dm-devel] [PATCH] dm-kcopyd: avoid useless atomic operations
@ 2021-05-25 19:49 Mikulas Patocka
2021-05-25 22:11 ` Damien Le Moal
0 siblings, 1 reply; 5+ messages in thread
From: Mikulas Patocka @ 2021-05-25 19:49 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel
The functions set_bit and clear_bit are atomic. We don't need atomicity
when making flags for dm-kcopyd. So, change them to direct manipulation of
the flags.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Index: linux-2.6/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-kcopyd.c
+++ linux-2.6/drivers/md/dm-kcopyd.c
@@ -812,7 +812,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli
if (!test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) {
for (i = 0; i < job->num_dests; i++) {
if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) {
- set_bit(DM_KCOPYD_WRITE_SEQ, &job->flags);
+ job->flags |= 1UL << DM_KCOPYD_WRITE_SEQ;
break;
}
}
@@ -823,7 +823,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli
*/
if (test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags) &&
test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags))
- clear_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags);
+ job->flags &= ~(1UL << DM_KCOPYD_IGNORE_ERROR);
if (from) {
job->source = *from;
Index: linux-2.6/drivers/md/dm-raid1.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-raid1.c
+++ linux-2.6/drivers/md/dm-raid1.c
@@ -364,7 +364,7 @@ static void recover(struct mirror_set *m
/* hand to kcopyd */
if (!errors_handled(ms))
- set_bit(DM_KCOPYD_IGNORE_ERROR, &flags);
+ flags |= 1UL << DM_KCOPYD_IGNORE_ERROR;
dm_kcopyd_copy(ms->kcopyd_client, &from, ms->nr_mirrors - 1, to,
flags, recovery_complete, reg);
Index: linux-2.6/drivers/md/dm-zoned-reclaim.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-zoned-reclaim.c
+++ linux-2.6/drivers/md/dm-zoned-reclaim.c
@@ -134,7 +134,7 @@ static int dmz_reclaim_copy(struct dmz_r
dst_zone_block = dmz_start_block(zmd, dst_zone);
if (dmz_is_seq(dst_zone))
- set_bit(DM_KCOPYD_WRITE_SEQ, &flags);
+ flags |= 1UL << DM_KCOPYD_WRITE_SEQ;
while (block < end_block) {
if (src_zone->dev->flags & DMZ_BDEV_DYING)
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [dm-devel] [PATCH] dm-kcopyd: avoid useless atomic operations 2021-05-25 19:49 [dm-devel] [PATCH] dm-kcopyd: avoid useless atomic operations Mikulas Patocka @ 2021-05-25 22:11 ` Damien Le Moal 2021-05-26 14:16 ` [dm-devel] [PATCH v2] " Mikulas Patocka 0 siblings, 1 reply; 5+ messages in thread From: Damien Le Moal @ 2021-05-25 22:11 UTC (permalink / raw) To: Mikulas Patocka, Mike Snitzer; +Cc: dm-devel@redhat.com On 2021/05/26 4:50, Mikulas Patocka wrote: > The functions set_bit and clear_bit are atomic. We don't need atomicity > when making flags for dm-kcopyd. So, change them to direct manipulation of > the flags. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Index: linux-2.6/drivers/md/dm-kcopyd.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-kcopyd.c > +++ linux-2.6/drivers/md/dm-kcopyd.c > @@ -812,7 +812,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > if (!test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { > for (i = 0; i < job->num_dests; i++) { > if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { > - set_bit(DM_KCOPYD_WRITE_SEQ, &job->flags); > + job->flags |= 1UL << DM_KCOPYD_WRITE_SEQ; How about using the BIT() macro ? job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); But I know some do not like that macro :) > break; > } > } > @@ -823,7 +823,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > */ > if (test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags) && > test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) > - clear_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags); > + job->flags &= ~(1UL << DM_KCOPYD_IGNORE_ERROR); > > if (from) { > job->source = *from; > Index: linux-2.6/drivers/md/dm-raid1.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-raid1.c > +++ linux-2.6/drivers/md/dm-raid1.c > @@ -364,7 +364,7 @@ static void recover(struct mirror_set *m > > /* hand to kcopyd */ > if (!errors_handled(ms)) > - set_bit(DM_KCOPYD_IGNORE_ERROR, &flags); > + flags |= 1UL << DM_KCOPYD_IGNORE_ERROR; > > dm_kcopyd_copy(ms->kcopyd_client, &from, ms->nr_mirrors - 1, to, > flags, recovery_complete, reg); > Index: linux-2.6/drivers/md/dm-zoned-reclaim.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-zoned-reclaim.c > +++ linux-2.6/drivers/md/dm-zoned-reclaim.c > @@ -134,7 +134,7 @@ static int dmz_reclaim_copy(struct dmz_r > dst_zone_block = dmz_start_block(zmd, dst_zone); > > if (dmz_is_seq(dst_zone)) > - set_bit(DM_KCOPYD_WRITE_SEQ, &flags); > + flags |= 1UL << DM_KCOPYD_WRITE_SEQ; > > while (block < end_block) { > if (src_zone->dev->flags & DMZ_BDEV_DYING) > > -- > dm-devel mailing list > dm-devel@redhat.com > https://listman.redhat.com/mailman/listinfo/dm-devel Either way, looks fine to me. Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com> -- Damien Le Moal Western Digital Research -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* [dm-devel] [PATCH v2] dm-kcopyd: avoid useless atomic operations 2021-05-25 22:11 ` Damien Le Moal @ 2021-05-26 14:16 ` Mikulas Patocka 2021-05-27 5:21 ` Damien Le Moal 0 siblings, 1 reply; 5+ messages in thread From: Mikulas Patocka @ 2021-05-26 14:16 UTC (permalink / raw) To: Damien Le Moal; +Cc: Mike Snitzer, dm-devel@redhat.com On Tue, 25 May 2021, Damien Le Moal wrote: > On 2021/05/26 4:50, Mikulas Patocka wrote: > > The functions set_bit and clear_bit are atomic. We don't need atomicity > > when making flags for dm-kcopyd. So, change them to direct manipulation of > > the flags. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > > Index: linux-2.6/drivers/md/dm-kcopyd.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-kcopyd.c > > +++ linux-2.6/drivers/md/dm-kcopyd.c > > @@ -812,7 +812,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > > if (!test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { > > for (i = 0; i < job->num_dests; i++) { > > if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { > > - set_bit(DM_KCOPYD_WRITE_SEQ, &job->flags); > > + job->flags |= 1UL << DM_KCOPYD_WRITE_SEQ; > > How about using the BIT() macro ? > > job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); > > But I know some do not like that macro :) Yes - it is better to use it. I also changed flags from unsigned long to unsigned, to make the structure smaller. From: Mikulas Patocka <mpatocka@redhat.com> dm-kcopyd: avoid useless atomic operations The functions set_bit and clear_bit are atomic. We don't need atomicity when making flags for dm-kcopyd. So, change them to direct manipulation of the flags. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Index: linux-2.6/drivers/md/dm-kcopyd.c =================================================================== --- linux-2.6.orig/drivers/md/dm-kcopyd.c +++ linux-2.6/drivers/md/dm-kcopyd.c @@ -341,7 +341,7 @@ static void client_free_pages(struct dm_ struct kcopyd_job { struct dm_kcopyd_client *kc; struct list_head list; - unsigned long flags; + unsigned flags; /* * Error state of the job. @@ -418,7 +418,7 @@ static struct kcopyd_job *pop_io_job(str * constraint and sequential writes that are at the right position. */ list_for_each_entry(job, jobs, list) { - if (job->rw == READ || !test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { + if (job->rw == READ || !(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { list_del(&job->list); return job; } @@ -529,7 +529,7 @@ static void complete_io(unsigned long er else job->read_err = 1; - if (!test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) { + if (!(job->flags & BIT(DM_KCOPYD_IGNORE_ERROR))) { push(&kc->complete_jobs, job); wake(kc); return; @@ -572,7 +572,7 @@ static int run_io_job(struct kcopyd_job * If we need to write sequentially and some reads or writes failed, * no point in continuing. */ - if (test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags) && + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && job->master_job->write_err) { job->write_err = job->master_job->write_err; return -EIO; @@ -716,7 +716,7 @@ static void segment_complete(int read_er * Only dispatch more work if there hasn't been an error. */ if ((!job->read_err && !job->write_err) || - test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) { + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) { /* get the next chunk of work */ progress = job->progress; count = job->source.count - progress; @@ -809,10 +809,10 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli * we need to write sequentially. If one of the destination is a * host-aware device, then leave it to the caller to choose what to do. */ - if (!test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { + if (!(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { for (i = 0; i < job->num_dests; i++) { if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { - set_bit(DM_KCOPYD_WRITE_SEQ, &job->flags); + job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); break; } } @@ -821,9 +821,9 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli /* * If we need to write sequentially, errors cannot be ignored. */ - if (test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags) && - test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) - clear_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags); + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) + job->flags &= ~BIT(DM_KCOPYD_IGNORE_ERROR); if (from) { job->source = *from; Index: linux-2.6/drivers/md/dm-raid1.c =================================================================== --- linux-2.6.orig/drivers/md/dm-raid1.c +++ linux-2.6/drivers/md/dm-raid1.c @@ -364,7 +364,7 @@ static void recover(struct mirror_set *m /* hand to kcopyd */ if (!errors_handled(ms)) - set_bit(DM_KCOPYD_IGNORE_ERROR, &flags); + flags |= BIT(DM_KCOPYD_IGNORE_ERROR); dm_kcopyd_copy(ms->kcopyd_client, &from, ms->nr_mirrors - 1, to, flags, recovery_complete, reg); Index: linux-2.6/drivers/md/dm-zoned-reclaim.c =================================================================== --- linux-2.6.orig/drivers/md/dm-zoned-reclaim.c +++ linux-2.6/drivers/md/dm-zoned-reclaim.c @@ -134,7 +134,7 @@ static int dmz_reclaim_copy(struct dmz_r dst_zone_block = dmz_start_block(zmd, dst_zone); if (dmz_is_seq(dst_zone)) - set_bit(DM_KCOPYD_WRITE_SEQ, &flags); + flags |= BIT(DM_KCOPYD_WRITE_SEQ); while (block < end_block) { if (src_zone->dev->flags & DMZ_BDEV_DYING) -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH v2] dm-kcopyd: avoid useless atomic operations 2021-05-26 14:16 ` [dm-devel] [PATCH v2] " Mikulas Patocka @ 2021-05-27 5:21 ` Damien Le Moal 2021-05-27 7:45 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Damien Le Moal @ 2021-05-27 5:21 UTC (permalink / raw) To: Mikulas Patocka; +Cc: Mike Snitzer, dm-devel@redhat.com On 2021/05/26 23:16, Mikulas Patocka wrote: > > > On Tue, 25 May 2021, Damien Le Moal wrote: > >> On 2021/05/26 4:50, Mikulas Patocka wrote: >>> The functions set_bit and clear_bit are atomic. We don't need atomicity >>> when making flags for dm-kcopyd. So, change them to direct manipulation of >>> the flags. >>> >>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> >>> >>> Index: linux-2.6/drivers/md/dm-kcopyd.c >>> =================================================================== >>> --- linux-2.6.orig/drivers/md/dm-kcopyd.c >>> +++ linux-2.6/drivers/md/dm-kcopyd.c >>> @@ -812,7 +812,7 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli >>> if (!test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { >>> for (i = 0; i < job->num_dests; i++) { >>> if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { >>> - set_bit(DM_KCOPYD_WRITE_SEQ, &job->flags); >>> + job->flags |= 1UL << DM_KCOPYD_WRITE_SEQ; >> >> How about using the BIT() macro ? >> >> job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); >> >> But I know some do not like that macro :) > > Yes - it is better to use it. > I also changed flags from unsigned long to unsigned, to make the structure > smaller. > > > From: Mikulas Patocka <mpatocka@redhat.com> > > dm-kcopyd: avoid useless atomic operations > > The functions set_bit and clear_bit are atomic. We don't need atomicity > when making flags for dm-kcopyd. So, change them to direct manipulation of > the flags. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Index: linux-2.6/drivers/md/dm-kcopyd.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-kcopyd.c > +++ linux-2.6/drivers/md/dm-kcopyd.c > @@ -341,7 +341,7 @@ static void client_free_pages(struct dm_ > struct kcopyd_job { > struct dm_kcopyd_client *kc; > struct list_head list; > - unsigned long flags; > + unsigned flags; This triggers a checkpatch warning. "unsigned int" would be better. > > /* > * Error state of the job. > @@ -418,7 +418,7 @@ static struct kcopyd_job *pop_io_job(str > * constraint and sequential writes that are at the right position. > */ > list_for_each_entry(job, jobs, list) { > - if (job->rw == READ || !test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { > + if (job->rw == READ || !(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { > list_del(&job->list); > return job; > } > @@ -529,7 +529,7 @@ static void complete_io(unsigned long er > else > job->read_err = 1; > > - if (!test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) { > + if (!(job->flags & BIT(DM_KCOPYD_IGNORE_ERROR))) { > push(&kc->complete_jobs, job); > wake(kc); > return; > @@ -572,7 +572,7 @@ static int run_io_job(struct kcopyd_job > * If we need to write sequentially and some reads or writes failed, > * no point in continuing. > */ > - if (test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags) && > + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && > job->master_job->write_err) { > job->write_err = job->master_job->write_err; > return -EIO; > @@ -716,7 +716,7 @@ static void segment_complete(int read_er > * Only dispatch more work if there hasn't been an error. > */ > if ((!job->read_err && !job->write_err) || > - test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) { > + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) { > /* get the next chunk of work */ > progress = job->progress; > count = job->source.count - progress; > @@ -809,10 +809,10 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > * we need to write sequentially. If one of the destination is a > * host-aware device, then leave it to the caller to choose what to do. > */ > - if (!test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags)) { > + if (!(job->flags & BIT(DM_KCOPYD_WRITE_SEQ))) { > for (i = 0; i < job->num_dests; i++) { > if (bdev_zoned_model(dests[i].bdev) == BLK_ZONED_HM) { > - set_bit(DM_KCOPYD_WRITE_SEQ, &job->flags); > + job->flags |= BIT(DM_KCOPYD_WRITE_SEQ); > break; > } > } > @@ -821,9 +821,9 @@ void dm_kcopyd_copy(struct dm_kcopyd_cli > /* > * If we need to write sequentially, errors cannot be ignored. > */ > - if (test_bit(DM_KCOPYD_WRITE_SEQ, &job->flags) && > - test_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags)) > - clear_bit(DM_KCOPYD_IGNORE_ERROR, &job->flags); > + if (job->flags & BIT(DM_KCOPYD_WRITE_SEQ) && > + job->flags & BIT(DM_KCOPYD_IGNORE_ERROR)) > + job->flags &= ~BIT(DM_KCOPYD_IGNORE_ERROR); > > if (from) { > job->source = *from; > Index: linux-2.6/drivers/md/dm-raid1.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-raid1.c > +++ linux-2.6/drivers/md/dm-raid1.c > @@ -364,7 +364,7 @@ static void recover(struct mirror_set *m > > /* hand to kcopyd */ > if (!errors_handled(ms)) > - set_bit(DM_KCOPYD_IGNORE_ERROR, &flags); > + flags |= BIT(DM_KCOPYD_IGNORE_ERROR); > > dm_kcopyd_copy(ms->kcopyd_client, &from, ms->nr_mirrors - 1, to, > flags, recovery_complete, reg); > Index: linux-2.6/drivers/md/dm-zoned-reclaim.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-zoned-reclaim.c > +++ linux-2.6/drivers/md/dm-zoned-reclaim.c > @@ -134,7 +134,7 @@ static int dmz_reclaim_copy(struct dmz_r > dst_zone_block = dmz_start_block(zmd, dst_zone); > > if (dmz_is_seq(dst_zone)) > - set_bit(DM_KCOPYD_WRITE_SEQ, &flags); > + flags |= BIT(DM_KCOPYD_WRITE_SEQ); > > while (block < end_block) { > if (src_zone->dev->flags & DMZ_BDEV_DYING) With the above nit corrected, Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com> -- Damien Le Moal Western Digital Research -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH v2] dm-kcopyd: avoid useless atomic operations 2021-05-27 5:21 ` Damien Le Moal @ 2021-05-27 7:45 ` Christoph Hellwig 0 siblings, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2021-05-27 7:45 UTC (permalink / raw) To: Damien Le Moal; +Cc: Mike Snitzer, Mikulas Patocka, dm-devel@redhat.com On Thu, May 27, 2021 at 05:21:24AM +0000, Damien Le Moal wrote: > > - unsigned long flags; > > + unsigned flags; > > This triggers a checkpatch warning. "unsigned int" would be better. No, it wouldn't. checkpatch is completely full of shit as usual. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-27 8:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-05-25 19:49 [dm-devel] [PATCH] dm-kcopyd: avoid useless atomic operations Mikulas Patocka 2021-05-25 22:11 ` Damien Le Moal 2021-05-26 14:16 ` [dm-devel] [PATCH v2] " Mikulas Patocka 2021-05-27 5:21 ` Damien Le Moal 2021-05-27 7:45 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).