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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> 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@xxxxxxx>
--
Damien Le Moal
Western Digital Research
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
