* [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
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
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
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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.