[PATCH 0/8] block: use right accessor to read nr_sects

[PATCH 0/8] block: use right accessor to read nr_sects

[Chaitanya Kulkarni]

Hi,

In the current block layer implementation including drivers and fs
there are some places in the code where block device->hd_part->number
of sectors is accessed directly without any locking. There is an
existing accessor function present in the include/linux/genhd.h
which should be used to read the bdev->hd_part->nr_sects
with the help of appropriate locking.

From ${KERN_DIR}/include/linux/genhd.h:-
<snip>
714 /*
715  * Any access of part->nr_sects which is not protected by partition
716  * bd_mutex or gendisk bdev bd_mutex, should be done using this
717  * accessor function.
718  *
719  * Code written along the lines of i_size_read() and i_size_write().
720  * CONFIG_PREEMPT case optimizes the case of UP kernel with preemption
721  * on.
722  */
723 static inline sector_t part_nr_sects_read(struct hd_struct *part)
724 {
<snip>

This patch series introduces a helper function on the top of the
part_nr_sects_read() with expected rcu locking and removes the direct
accesses to the bdev->hd_part->nr_sects.

This is based on :-
1. Repo :- git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git.
2. Branch :- for-next.

Regards,
Chaitanya

Chaitanya Kulkarni (8):
  block: add a helper function to read nr_setcs
  blk-zoned: update blkdev_nr_zones() with helper
  blk-zoned: update blkdev_report_zone() with helper
  blk-zoned: update blkdev_reset_zones() with helper
  bcache: update cached_dev_init() with helper
  target/pscsi: use helper in pscsi_get_blocks()
  f2fs: use helper in init_blkz_info()
  blktrace: use helper in blk_trace_setup_lba()

 block/blk-zoned.c                  | 12 ++++++------
 drivers/md/bcache/super.c          |  2 +-
 drivers/target/target_core_pscsi.c |  2 +-
 fs/f2fs/super.c                    |  2 +-
 include/linux/blkdev.h             | 12 ++++++++++++
 kernel/trace/blktrace.c            |  2 +-
 6 files changed, 22 insertions(+), 10 deletions(-)

-- 
2.19.1

[PATCH 1/8] block: add a helper function to read nr_setcs

[Chaitanya Kulkarni]

This patch introduces helper function to read the number of sectors
from struct block_device->bd_part member. For more details Please refer
to the comment in the include/linux/genhd.h for part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/linux/blkdev.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 592669bcc536..1ae65107182a 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1475,6 +1475,18 @@ static inline void put_dev_sector(Sector p)
 	put_page(p.v);
 }
 
+/* Helper function to read the bdev->bd_part->nr_sects */
+static inline sector_t bdev_nr_sects(struct block_device *bdev)
+{
+	sector_t nr_sects;
+
+	rcu_read_lock();
+	nr_sects = part_nr_sects_read(bdev->bd_part);
+	rcu_read_unlock();
+
+	return nr_sects;
+}
+
 int kblockd_schedule_work(struct work_struct *work);
 int kblockd_schedule_work_on(int cpu, struct work_struct *work);
 int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork, unsigned long delay);
-- 
2.19.1

[PATCH 2/8] blk-zoned: update blkdev_nr_zones() with helper

[Chaitanya Kulkarni]

This patch updates the blkdev_nr_zones() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-zoned.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index ae7e91bd0618..5051db35c3fd 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -90,7 +90,7 @@ unsigned int blkdev_nr_zones(struct block_device *bdev)
 	if (!blk_queue_is_zoned(q))
 		return 0;
 
-	return __blkdev_nr_zones(q, bdev->bd_part->nr_sects);
+	return __blkdev_nr_zones(q, bdev_nr_sects(bdev));
 }
 EXPORT_SYMBOL_GPL(blkdev_nr_zones);
 
-- 
2.19.1

[PATCH 3/8] blk-zoned: update blkdev_report_zone() with helper

[Chaitanya Kulkarni]

This patch updates the blkdev_report_zone(s)() with newly introduced
helper function to read the nr_sects from block device's hd_parts with
the help of part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-zoned.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 5051db35c3fd..9faf4488339d 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -106,7 +106,7 @@ static bool blkdev_report_zone(struct block_device *bdev, struct blk_zone *rep)
 		return false;
 
 	rep->start -= offset;
-	if (rep->start + rep->len > bdev->bd_part->nr_sects)
+	if (rep->start + rep->len > bdev_nr_sects(bdev))
 		return false;
 
 	if (rep->type == BLK_ZONE_TYPE_CONVENTIONAL)
@@ -176,13 +176,13 @@ int blkdev_report_zones(struct block_device *bdev, sector_t sector,
 	if (WARN_ON_ONCE(!bdev->bd_disk->fops->report_zones))
 		return -EOPNOTSUPP;
 
-	if (!*nr_zones || sector >= bdev->bd_part->nr_sects) {
+	if (!*nr_zones || sector >= bdev_nr_sects(bdev)) {
 		*nr_zones = 0;
 		return 0;
 	}
 
 	nrz = min(*nr_zones,
-		  __blkdev_nr_zones(q, bdev->bd_part->nr_sects - sector));
+		  __blkdev_nr_zones(q, bdev_nr_sects(bdev) - sector));
 	ret = blk_report_zones(bdev->bd_disk, get_start_sect(bdev) + sector,
 			       zones, &nrz, gfp_mask);
 	if (ret)
-- 
2.19.1

[PATCH 4/8] blk-zoned: update blkdev_reset_zones() with helper

[Chaitanya Kulkarni]

This patch updates the blkdev_reset_zones() with newly introduced
helper function to read the nr_sects from block device's hd_parts with
the help of part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-zoned.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 9faf4488339d..e7f2874b5d37 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -229,7 +229,7 @@ int blkdev_reset_zones(struct block_device *bdev,
 	if (bdev_read_only(bdev))
 		return -EPERM;
 
-	if (!nr_sectors || end_sector > bdev->bd_part->nr_sects)
+	if (!nr_sectors || end_sector > bdev_nr_sects(bdev))
 		/* Out of range */
 		return -EINVAL;
 
@@ -239,7 +239,7 @@ int blkdev_reset_zones(struct block_device *bdev,
 		return -EINVAL;
 
 	if ((nr_sectors & (zone_sectors - 1)) &&
-	    end_sector != bdev->bd_part->nr_sects)
+	    end_sector != bdev_nr_sects(bdev))
 		return -EINVAL;
 
 	blk_start_plug(&plug);
-- 
2.19.1

[PATCH 5/8] bcache: update cached_dev_init() with helper

[Chaitanya Kulkarni]

In the bcache when initializing the cached device we don't actually
use any sort of locking when reading the number of sectors from the
part. This patch updates the cached_dev_init() with newly introduced
helper function to read the nr_sects from block device's hd_parts with
the help if part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/md/bcache/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 1b63ac876169..6a29ba89dae1 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1263,7 +1263,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
 			q->limits.raid_partial_stripes_expensive;
 
 	ret = bcache_device_init(&dc->disk, block_size,
-			 dc->bdev->bd_part->nr_sects - dc->sb.data_offset);
+			 bdev_nr_sects(dc->bdev) - dc->sb.data_offset);
 	if (ret)
 		return ret;
 
-- 
2.19.1

[COMPILE TESTED PATCH 6/8] target/pscsi: use helper in pscsi_get_blocks()

[Chaitanya Kulkarni]

This patch updates the pscsi_get_blocks() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/target/target_core_pscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index c9d92b3e777d..da481edab2de 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1030,7 +1030,7 @@ static sector_t pscsi_get_blocks(struct se_device *dev)
 	struct pscsi_dev_virt *pdv = PSCSI_DEV(dev);
 
 	if (pdv->pdv_bd && pdv->pdv_bd->bd_part)
-		return pdv->pdv_bd->bd_part->nr_sects;
+		return bdev_nr_sects(pdv->pdv_bd);
 
 	return 0;
 }
-- 
2.19.1

[COMPILE TESTED PATCH 7/8] f2fs: use helper in init_blkz_info()

[Chaitanya Kulkarni]

This patch updates the init_blkz_info() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 fs/f2fs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 6b959bbb336a..24e2848afcf5 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2798,7 +2798,7 @@ static int init_percpu_info(struct f2fs_sb_info *sbi)
 static int init_blkz_info(struct f2fs_sb_info *sbi, int devi)
 {
 	struct block_device *bdev = FDEV(devi).bdev;
-	sector_t nr_sectors = bdev->bd_part->nr_sects;
+	sector_t nr_sectors = bdev_nr_sects(bdev);
 	sector_t sector = 0;
 	struct blk_zone *zones;
 	unsigned int i, nr_zones;
-- 
2.19.1

[PATCH 8/8] blktrace: use helper in blk_trace_setup_lba()

[Chaitanya Kulkarni]

This patch updates the blk_trace_setup_lba() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read() protected by appropriate locking.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 kernel/trace/blktrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index e1c6d79fb4cc..35ff49503b85 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -461,7 +461,7 @@ static void blk_trace_setup_lba(struct blk_trace *bt,
 
 	if (part) {
 		bt->start_lba = part->start_sect;
-		bt->end_lba = part->start_sect + part->nr_sects;
+		bt->end_lba = part->start_sect + bdev_nr_sects(bdev);
 	} else {
 		bt->start_lba = 0;
 		bt->end_lba = -1ULL;
-- 
2.19.1

Re: [PATCH 1/8] block: add a helper function to read nr_setcs

[Bart Van Assche]

On 6/13/19 7:59 AM, Chaitanya Kulkarni wrote:
> This patch introduces helper function to read the number of sectors
> from struct block_device->bd_part member. For more details Please refer
> to the comment in the include/linux/genhd.h for part_nr_sects_read().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>   include/linux/blkdev.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 592669bcc536..1ae65107182a 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1475,6 +1475,18 @@ static inline void put_dev_sector(Sector p)
>   	put_page(p.v);
>   }
>   
> +/* Helper function to read the bdev->bd_part->nr_sects */
> +static inline sector_t bdev_nr_sects(struct block_device *bdev)
> +{
> +	sector_t nr_sects;
> +
> +	rcu_read_lock();
> +	nr_sects = part_nr_sects_read(bdev->bd_part);
> +	rcu_read_unlock();
> +
> +	return nr_sects;
> +}
> +
>   int kblockd_schedule_work(struct work_struct *work);
>   int kblockd_schedule_work_on(int cpu, struct work_struct *work);
>   int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork, unsigned long delay);
> 

Please explain what makes you think that part_nr_sects_read() must be 
protected by an RCU read lock.

Thanks,

Bart.

Re: [COMPILE TESTED PATCH 6/8] target/pscsi: use helper in pscsi_get_blocks()

[Bart Van Assche]

On 6/13/19 7:59 AM, Chaitanya Kulkarni wrote:
> This patch updates the pscsi_get_blocks() with newly introduced helper
> function to read the nr_sects from block device's hd_parts with the
> help if part_nr_sects_read() protected by appropriate locking.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>   drivers/target/target_core_pscsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index c9d92b3e777d..da481edab2de 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -1030,7 +1030,7 @@ static sector_t pscsi_get_blocks(struct se_device *dev)
>   	struct pscsi_dev_virt *pdv = PSCSI_DEV(dev);
>   
>   	if (pdv->pdv_bd && pdv->pdv_bd->bd_part)
> -		return pdv->pdv_bd->bd_part->nr_sects;
> +		return bdev_nr_sects(pdv->pdv_bd);
>   
>   	return 0;
>   }

As far as I can see bd_part does not change between blkdev_get() and 
blkdev_put(). Since the pscsi code guarantees that blkdev_put() is not 
called concurrently with pscsi_get_blocks() this patch is not necessary.

Bart.

Re: [PATCH 1/8] block: add a helper function to read nr_setcs

[Douglas Gilbert]

On 2019-06-13 11:31 a.m., Bart Van Assche wrote:
> On 6/13/19 7:59 AM, Chaitanya Kulkarni wrote:
>> This patch introduces helper function to read the number of sectors
>> from struct block_device->bd_part member. For more details Please refer
>> to the comment in the include/linux/genhd.h for part_nr_sects_read().
>>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>   include/linux/blkdev.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 592669bcc536..1ae65107182a 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -1475,6 +1475,18 @@ static inline void put_dev_sector(Sector p)
>>       put_page(p.v);
>>   }
>> +/* Helper function to read the bdev->bd_part->nr_sects */
>> +static inline sector_t bdev_nr_sects(struct block_device *bdev)
>> +{
>> +    sector_t nr_sects;
>> +
>> +    rcu_read_lock();
>> +    nr_sects = part_nr_sects_read(bdev->bd_part);
>> +    rcu_read_unlock();
>> +
>> +    return nr_sects;
>> +}
>> +
>>   int kblockd_schedule_work(struct work_struct *work);
>>   int kblockd_schedule_work_on(int cpu, struct work_struct *work);
>>   int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork, 
>> unsigned long delay);
>>
> 
> Please explain what makes you think that part_nr_sects_read() must be protected 
> by an RCU read lock.

Dear reviewer,
Please rephrase the above sentence without the accusative tone.
Specifically, please do not use the phrase "what makes you think"
in this or any other code review. For example: "I believe that..."
is more accurate and less provocative.


Observation: as a Canadian citizen when crossing the US border I
believe contradicting a US border official with the phrase "what
makes you think ..." could lead to a rather bad outcome :-)
Please make review comments with that in mind.

Thanks.

Doug Gilbert

P.S. Do we have any Linux code-of-conduct for reviewers?

Re: [PATCH 1/8] block: add a helper function to read nr_setcs

[Chaitanya Kulkarni]

On 06/13/2019 08:31 AM, Bart Van Assche wrote:
> On 6/13/19 7:59 AM, Chaitanya Kulkarni wrote:
>> This patch introduces helper function to read the number of sectors
>> from struct block_device->bd_part member. For more details Please refer
>> to the comment in the include/linux/genhd.h for part_nr_sects_read().
>>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>    include/linux/blkdev.h | 12 ++++++++++++
>>    1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 592669bcc536..1ae65107182a 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -1475,6 +1475,18 @@ static inline void put_dev_sector(Sector p)
>>    	put_page(p.v);
>>    }
>>
>> +/* Helper function to read the bdev->bd_part->nr_sects */
>> +static inline sector_t bdev_nr_sects(struct block_device *bdev)
>> +{
>> +	sector_t nr_sects;
>> +
>> +	rcu_read_lock();
>> +	nr_sects = part_nr_sects_read(bdev->bd_part);
>> +	rcu_read_unlock();
>> +
>> +	return nr_sects;
>> +}
>> +
>>    int kblockd_schedule_work(struct work_struct *work);
>>    int kblockd_schedule_work_on(int cpu, struct work_struct *work);
>>    int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork, unsigned long delay);
>>
>
> Please explain what makes you think that part_nr_sects_read() must be
> protected by an RCU read lock.

Thanks Bart for looking into this, we actually don't need those locking.

I'll fix this in the V2.

Please let me know if you find any other issues with this series.

>
> Thanks,
>
> Bart.
>

Re: [COMPILE TESTED PATCH 6/8] target/pscsi: use helper in pscsi_get_blocks()

[Chaitanya Kulkarni]

Okay, I'll drop this patch in the V2.

On 06/13/2019 08:36 AM, Bart Van Assche wrote:
> On 6/13/19 7:59 AM, Chaitanya Kulkarni wrote:
>> This patch updates the pscsi_get_blocks() with newly introduced helper
>> function to read the nr_sects from block device's hd_parts with the
>> help if part_nr_sects_read() protected by appropriate locking.
>>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>    drivers/target/target_core_pscsi.c | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
>> index c9d92b3e777d..da481edab2de 100644
>> --- a/drivers/target/target_core_pscsi.c
>> +++ b/drivers/target/target_core_pscsi.c
>> @@ -1030,7 +1030,7 @@ static sector_t pscsi_get_blocks(struct se_device *dev)
>>    	struct pscsi_dev_virt *pdv = PSCSI_DEV(dev);
>>
>>    	if (pdv->pdv_bd && pdv->pdv_bd->bd_part)
>> -		return pdv->pdv_bd->bd_part->nr_sects;
>> +		return bdev_nr_sects(pdv->pdv_bd);
>>
>>    	return 0;
>>    }
>
> As far as I can see bd_part does not change between blkdev_get() and
> blkdev_put(). Since the pscsi code guarantees that blkdev_put() is not
> called concurrently with pscsi_get_blocks() this patch is not necessary.
>
> Bart.
>
>
>

Re: [PATCH 1/8] block: add a helper function to read nr_setcs

[James Bottomley]

On Thu, 2019-06-13 at 12:07 -0400, Douglas Gilbert wrote:
> On 2019-06-13 11:31 a.m., Bart Van Assche wrote:
[...]
> > Please explain what makes you think that part_nr_sects_read() must
> > be protected 
> > by an RCU read lock.
> 
> Dear reviewer,
> Please rephrase the above sentence without the accusative tone.
> Specifically, please do not use the phrase "what makes you think"
> in this or any other code review. For example: "I believe that..."
> is more accurate and less provocative.

Imputing "tone" to email is something we try to avoid because it never
ends well, particularly for non-native speakers. Some languages
(Russian) have no articles and if you take any English phrase and strip
out all the articles it sounds a lot more aggressive.

> Observation: as a Canadian citizen when crossing the US border I
> believe contradicting a US border official with the phrase "what
> makes you think ..." could lead to a rather bad outcome :-)
> Please make review comments with that in mind.

Different situation: we aren't profiling reviewers ...

> Thanks.
> 
> Doug Gilbert
> 
> P.S. Do we have any Linux code-of-conduct for reviewers?

It's the same one for all interactions:

Documentation/process/code-of-conduct-interpretation.rst

But I would remind everyone that diversity isn't just a
gender/race/LGBT issue it also means being understanding of the
potential difficulties non-native speakers have with email in English.

James

Re: [PATCH 1/8] block: add a helper function to read nr_setcs

[Douglas Gilbert]

On 2019-06-13 12:28 p.m., James Bottomley wrote:
> On Thu, 2019-06-13 at 12:07 -0400, Douglas Gilbert wrote:
>> On 2019-06-13 11:31 a.m., Bart Van Assche wrote:
> [...]
>>> Please explain what makes you think that part_nr_sects_read() must
>>> be protected
>>> by an RCU read lock.
>>
>> Dear reviewer,
>> Please rephrase the above sentence without the accusative tone.
>> Specifically, please do not use the phrase "what makes you think"
>> in this or any other code review. For example: "I believe that..."
>> is more accurate and less provocative.
> 
> Imputing "tone" to email is something we try to avoid because it never
> ends well, particularly for non-native speakers. Some languages
> (Russian) have no articles and if you take any English phrase and strip
> out all the articles it sounds a lot more aggressive.

Like you, I am not a native North American English speaker but I
have lived here long enough to realize that "what makes you think ..."
is not a pleasantry and it may be fishing for an emotive
reaction. It is not the type of expression that professionals would
use to make a point in a public forum.

I'm not talking about articles (e.g. "a" and "the"), I'm talking
about pronouns like "you" and "I". I'm not aware of any languages
without pronouns. IMO Bart uses expressions with "you" in them too
often when he is expressing _his_ opinion to the contrary.

>> Observation: as a Canadian citizen when crossing the US border I
>> believe contradicting a US border official with the phrase "what
>> makes you think ..." could lead to a rather bad outcome :-)
>> Please make review comments with that in mind.
> 
> Different situation: we aren't profiling reviewers ...

Would you have used that expression when addressing a teacher at
high school or university? I'm looking for a yardstick of where
a reviewer should "pitch" their responses. The way you address
someone who has the ability to make your life uncomfortable (e.g.
by refusing you entry into their country) may just be such a
yardstick.

>> P.S. Do we have any Linux code-of-conduct for reviewers?
> 
> It's the same one for all interactions:
> 
> Documentation/process/code-of-conduct-interpretation.rst
> 
> But I would remind everyone that diversity isn't just a
> gender/race/LGBT issue it also means being understanding of the
> potential difficulties non-native speakers have with email in English.

To quote
   https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
to which your above reference indirectly refers:

    It calls for a "harassment-free experience for everyone,
    regardless of ... expression ..."

So informing someone (not for the first time) that readers of the
language in which they are writing, may take offence at their
expression is: not showing an "understanding of the potential
difficulties non-native speakers have" and thus is harassment?
Balance that with the angle of a reviewer trying to intimidate
the person presenting the code. Could that also be harassment?
In this case I see little evidence of the "potential difficulties"
to which you refer.


More generally:
IMO those who have power speak in a condescending fashion and act
unilaterally in the matter of reviewing and applying patches. A
select few are allowed to apply patches seemingly without any
review and ignore error reports or attempts at public review.
It certainly does not look like a system based on merit.

Doug Gilbert

Re: [PATCH 1/8] block: add a helper function to read nr_setcs

[James Bottomley]

On Thu, 2019-06-13 at 15:26 -0400, Douglas Gilbert wrote:
> On 2019-06-13 12:28 p.m., James Bottomley wrote:
> > On Thu, 2019-06-13 at 12:07 -0400, Douglas Gilbert wrote:
> > > On 2019-06-13 11:31 a.m., Bart Van Assche wrote:
> > 
> > [...]
> > > > Please explain what makes you think that part_nr_sects_read()
> > > > must
> > > > be protected
> > > > by an RCU read lock.
> > > 
> > > Dear reviewer,
> > > Please rephrase the above sentence without the accusative tone.
> > > Specifically, please do not use the phrase "what makes you think"
> > > in this or any other code review. For example: "I believe
> > > that..." is more accurate and less provocative.
> > 
> > Imputing "tone" to email is something we try to avoid because it
> > never ends well, particularly for non-native speakers. Some
> > languages (Russian) have no articles and if you take any English
> > phrase and strip out all the articles it sounds a lot more
> > aggressive.
> 
> Like you, I am not a native North American English speaker but I
> have lived here long enough to realize that "what makes you think
> ..." is not a pleasantry and it may be fishing for an emotive
> reaction. It is not the type of expression that professionals would
> use to make a point in a public forum.

I'm not so sure of that, for instance what makes you think I don't do
it in my own reviews?

> I'm not talking about articles (e.g. "a" and "the"), I'm talking
> about pronouns like "you" and "I". I'm not aware of any languages
> without pronouns. IMO Bart uses expressions with "you" in them too
> often when he is expressing _his_ opinion to the contrary.

It's a grammatical tick not an insult and seeing it as such would help
defuse the situation.  I know this is difficult; my own pet grammatical
foible is having to contain it when I see "avoid that" in a patch
subject, but I've managed (so far).

> > > Observation: as a Canadian citizen when crossing the US border I
> > > believe contradicting a US border official with the phrase "what
> > > makes you think ..." could lead to a rather bad outcome :-)
> > > Please make review comments with that in mind.
> > 
> > Different situation: we aren't profiling reviewers ...
> 
> Would you have used that expression when addressing a teacher at
> high school or university? I'm looking for a yardstick of where
> a reviewer should "pitch" their responses. The way you address
> someone who has the ability to make your life uncomfortable (e.g.
> by refusing you entry into their country) may just be such a
> yardstick.
> 
> > > P.S. Do we have any Linux code-of-conduct for reviewers?
> > 
> > It's the same one for all interactions:
> > 
> > Documentation/process/code-of-conduct-interpretation.rst
> > 
> > But I would remind everyone that diversity isn't just a
> > gender/race/LGBT issue it also means being understanding of the
> > potential difficulties non-native speakers have with email in
> > English.
> 
> To quote
>    https://www.contributor-covenant.org/version/1/4/code-of-conduct.h
> tml
> to which your above reference indirectly refers:
> 
>     It calls for a "harassment-free experience for everyone,
>     regardless of ... expression ..."

OK, we picked a code of conduct which is Anglo biased and doesn't take
into account the linguistic diversity of the community; the various
problems with the current code of conduct are why we have to have the
interpretation document.

> So informing someone (not for the first time) that readers of the
> language in which they are writing, may take offence at their
> expression is: not showing an "understanding of the potential
> difficulties non-native speakers have" and thus is harassment?
> Balance that with the angle of a reviewer trying to intimidate
> the person presenting the code. Could that also be harassment?
> In this case I see little evidence of the "potential difficulties"
> to which you refer.

The problem, as I see it, is that you're assuming malice where I
wouldn't, even if linguistic issues weren't a potential issue.

> More generally:
> IMO those who have power speak in a condescending fashion and act
> unilaterally in the matter of reviewing and applying patches. A
> select few are allowed to apply patches seemingly without any
> review and ignore error reports or attempts at public review.
> It certainly does not look like a system based on merit.

Is there, perhaps, some other deeper underlying issue for which this is
serving as a proxy?

James