| Message ID | 20190613145955.4813-2-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: use right accessor to read nr_sects | expand |
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.
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?
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. >
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
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
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
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);
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(+)