[1/8] block: add a helper function to read nr_setcs
diff mbox series

Message ID 20190613145955.4813-2-chaitanya.kulkarni@wdc.com
State New
Headers show
Series
  • block: use right accessor to read nr_sects
Related show

Commit Message

Chaitanya Kulkarni June 13, 2019, 2:59 p.m. UTC
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(+)

Comments

Bart Van Assche June 13, 2019, 3:31 p.m. UTC | #1
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.
Douglas Gilbert June 13, 2019, 4:07 p.m. UTC | #2
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?
Chaitanya Kulkarni June 13, 2019, 4:07 p.m. UTC | #3
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.
>
James Bottomley June 13, 2019, 4:28 p.m. UTC | #4
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
Douglas Gilbert June 13, 2019, 7:26 p.m. UTC | #5
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
James Bottomley June 13, 2019, 8:29 p.m. UTC | #6
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

Patch
diff mbox series

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);