[dm-devel] [PATCH v2] dm-kcopyd: avoid useless atomic operations

[Damien Le Moal]

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 at 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 at 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 at 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 at wdc.com>


-- 
Damien Le Moal
Western Digital Research