There are a few uses of an ugly ternary operator in raid5_make_request()
to check if a sector is a head of a reshape sector.
Factor this out into a simple helper called ahead_of_reshape().
This appears to fix the first bio_wouldblock_error() check which appears
to have comparison operators that didn't match the check below which
causes a schedule. Besides this, no functional changes intended.
Suggested-by: Christoph Hellwig <[email protected]>
Signed-off-by: Logan Gunthorpe <[email protected]>
---
drivers/md/raid5.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7f7d1546b9ba..97b23c18402b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5787,6 +5787,15 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
bio_endio(bi);
}
+static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
+ sector_t reshape_sector)
+{
+ if (mddev->reshape_backwards)
+ return sector < reshape_sector;
+ else
+ return sector >= reshape_sector;
+}
+
static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
{
struct r5conf *conf = mddev->private;
@@ -5843,9 +5852,8 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
/* Bail out if conflicts with reshape and REQ_NOWAIT is set */
if ((bi->bi_opf & REQ_NOWAIT) &&
(conf->reshape_progress != MaxSector) &&
- (mddev->reshape_backwards
- ? (logical_sector > conf->reshape_progress && logical_sector <= conf->reshape_safe)
- : (logical_sector >= conf->reshape_safe && logical_sector < conf->reshape_progress))) {
+ !ahead_of_reshape(mddev, logical_sector, conf->reshape_progress) &&
+ ahead_of_reshape(mddev, logical_sector, conf->reshape_safe)) {
bio_wouldblock_error(bi);
if (rw == WRITE)
md_write_end(mddev);
@@ -5874,14 +5882,12 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
* to check again.
*/
spin_lock_irq(&conf->device_lock);
- if (mddev->reshape_backwards
- ? logical_sector < conf->reshape_progress
- : logical_sector >= conf->reshape_progress) {
+ if (ahead_of_reshape(mddev, logical_sector,
+ conf->reshape_progress)) {
previous = 1;
} else {
- if (mddev->reshape_backwards
- ? logical_sector < conf->reshape_safe
- : logical_sector >= conf->reshape_safe) {
+ if (ahead_of_reshape(mddev, logical_sector,
+ conf->reshape_safe)) {
spin_unlock_irq(&conf->device_lock);
schedule();
do_prepare = true;
@@ -5912,9 +5918,8 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
*/
int must_retry = 0;
spin_lock_irq(&conf->device_lock);
- if (mddev->reshape_backwards
- ? logical_sector >= conf->reshape_progress
- : logical_sector < conf->reshape_progress)
+ if (!ahead_of_reshape(mddev, logical_sector,
+ conf->reshape_progress))
/* mismatch, need to try again */
must_retry = 1;
spin_unlock_irq(&conf->device_lock);
--
2.30.2
On 21/04/2022 17:05, Logan Gunthorpe wrote:
>> I like the ternary operator.;-)
>>
>> return mddev->reshape_backwards ? (return sector < reshape_sector) :
>> (sector >= reshape_sector)
>>
>> Sorry, does not matter.
> Yeah, I think plenty of people do not, though; it's harder to read with
> the long line and awkward to wrap.
>
I like the ternary too, but is there a superfluous return in there? That
would shorten the line. Or break it on the question mark - condition,
true, false all on their own lines.
Cheers,
Wol
On Wed, Apr 20, 2022 at 01:54:14PM -0600, Logan Gunthorpe wrote:
> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
> + sector_t reshape_sector)
> +{
> + if (mddev->reshape_backwards)
> + return sector < reshape_sector;
> + else
> + return sector >= reshape_sector;
> +}
No real eed for the else here. Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On 2022-04-21 03:17, Paul Menzel wrote:
> Dear Logan,
>
>
> Thank you for these patches.
>
>
> Am 20.04.22 um 21:54 schrieb Logan Gunthorpe:
>> There are a few uses of an ugly ternary operator in raid5_make_request()
>> to check if a sector is a head of a reshape sector.
>>
>> Factor this out into a simple helper called ahead_of_reshape().
>>
>> This appears to fix the first bio_wouldblock_error() check which appears
>> to have comparison operators that didn't match the check below which
>> causes a schedule. Besides this, no functional changes intended.
>
> If there is an error, could that be fixed in a separate commit, which
> could be applied to the stable series?
Yes, sure. Though, I'm not 100% sure there's an error or noticeable bug.
It's just that the logic didn't match and it made cleaning up the code
overly complicated.
>
>> Suggested-by: Christoph Hellwig <[email protected]>
>> Signed-off-by: Logan Gunthorpe <[email protected]>
>> ---
>> drivers/md/raid5.c | 29 +++++++++++++++++------------
>> 1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 7f7d1546b9ba..97b23c18402b 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5787,6 +5787,15 @@ static void make_discard_request(struct mddev
>> *mddev, struct bio *bi)
>> bio_endio(bi);
>> }
>> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
>> + sector_t reshape_sector)
>> +{
>> + if (mddev->reshape_backwards)
>> + return sector < reshape_sector;
>> + else
>> + return sector >= reshape_sector;
>
> I like the ternary operator. ;-)
>
> return mddev->reshape_backwards ? (return sector < reshape_sector) :
> (sector >= reshape_sector)
>
> Sorry, does not matter.
Yeah, I think plenty of people do not, though; it's harder to read with
the long line and awkward to wrap.
> Reviewed-by: Paul Menzel <[email protected]>
Thanks,
Logan
On 4/21/22 3:54 AM, Logan Gunthorpe wrote:
> There are a few uses of an ugly ternary operator in raid5_make_request()
> to check if a sector is a head of a reshape sector.
>
> Factor this out into a simple helper called ahead_of_reshape().
>
> This appears to fix the first bio_wouldblock_error() check which appears
> to have comparison operators that didn't match the check below which
> causes a schedule. Besides this, no functional changes intended.
>
> Suggested-by: Christoph Hellwig<[email protected]>
> Signed-off-by: Logan Gunthorpe<[email protected]>
> ---
> drivers/md/raid5.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 7f7d1546b9ba..97b23c18402b 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5787,6 +5787,15 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
> bio_endio(bi);
> }
>
> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
> + sector_t reshape_sector)
> +{
> + if (mddev->reshape_backwards)
> + return sector < reshape_sector;
> + else
> + return sector >= reshape_sector;
> +}
I think it can be an inline function.
> +
> static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
> {
> struct r5conf *conf = mddev->private;
> @@ -5843,9 +5852,8 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
> /* Bail out if conflicts with reshape and REQ_NOWAIT is set */
> if ((bi->bi_opf & REQ_NOWAIT) &&
> (conf->reshape_progress != MaxSector) &&
> - (mddev->reshape_backwards
> - ? (logical_sector > conf->reshape_progress && logical_sector <= conf->reshape_safe)
> - : (logical_sector >= conf->reshape_safe && logical_sector < conf->reshape_progress))) {
> + !ahead_of_reshape(mddev, logical_sector, conf->reshape_progress) &&
> + ahead_of_reshape(mddev, logical_sector, conf->reshape_safe)) {
TBH, the previous code is more readable to me though I can live with the
change.
Thanks,
Guoqing
On 2022-04-26 19:28, Guoqing Jiang wrote:
>> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
>> + sector_t reshape_sector)
>> +{
>> + if (mddev->reshape_backwards)
>> + return sector < reshape_sector;
>> + else
>> + return sector >= reshape_sector;
>> +}
>
> I think it can be an inline function.
Marking static functions in C files as inline is not recommended. GCC
will inline it, if it is appropriate.
https://yarchive.net/comp/linux/inline.html
https://www.kernel.org/doc/local/inline.html
Logan
On 4/28/22 12:07 AM, Logan Gunthorpe wrote:
>
> On 2022-04-26 19:28, Guoqing Jiang wrote:
>>> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
>>> + sector_t reshape_sector)
>>> +{
>>> + if (mddev->reshape_backwards)
>>> + return sector < reshape_sector;
>>> + else
>>> + return sector >= reshape_sector;
>>> +}
>> I think it can be an inline function.
> Marking static functions in C files as inline is not recommended. GCC
> will inline it, if it is appropriate.
>
> https://yarchive.net/comp/linux/inline.html
> https://www.kernel.org/doc/local/inline.html
Thanks for the link, then I suppose those can be deleted
linux> grep "static inline" drivers/md/md.h -r
static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
static inline int __must_check mddev_lock(struct mddev *mddev)
static inline void mddev_lock_nointr(struct mddev *mddev)
static inline int mddev_trylock(struct mddev *mddev)
static inline int mddev_is_locked(struct mddev *mddev)
static inline void md_sync_acct(struct block_device *bdev, unsigned long
nr_sectors)
static inline void md_sync_acct_bio(struct bio *bio, unsigned long
nr_sectors)
static inline struct kernfs_node *sysfs_get_dirent_safe(struct
kernfs_node *sd, char *name)
static inline void sysfs_notify_dirent_safe(struct kernfs_node *sd)
static inline char * mdname (struct mddev * mddev)
static inline int sysfs_link_rdev(struct mddev *mddev, struct md_rdev *rdev)
static inline void sysfs_unlink_rdev(struct mddev *mddev, struct md_rdev
*rdev)
static inline void safe_put_page(struct page *p)
static inline bool is_mddev_broken(struct md_rdev *rdev, const char
*md_type)
static inline void rdev_dec_pending(struct md_rdev *rdev, struct mddev
*mddev)
static inline int mddev_is_clustered(struct mddev *mddev)
static inline void mddev_clear_unsupported_flags(struct mddev *mddev,
static inline void mddev_check_write_zeroes(struct mddev *mddev, struct
bio *bio)
Thanks,
Guoqing
On 2022-04-27 19:49, Guoqing Jiang wrote:
>
>
> On 4/28/22 12:07 AM, Logan Gunthorpe wrote:
>>
>> On 2022-04-26 19:28, Guoqing Jiang wrote:
>>>> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
>>>> + sector_t reshape_sector)
>>>> +{
>>>> + if (mddev->reshape_backwards)
>>>> + return sector < reshape_sector;
>>>> + else
>>>> + return sector >= reshape_sector;
>>>> +}
>>> I think it can be an inline function.
>> Marking static functions in C files as inline is not recommended. GCC
>> will inline it, if it is appropriate.
>>
>> https://yarchive.net/comp/linux/inline.html
>> https://www.kernel.org/doc/local/inline.html
>
> Thanks for the link, then I suppose those can be deleted
> linux> grep "static inline" drivers/md/md.h -r
It's standard practice to annotate small functions in headers with
"static inline". Without the inline annotation, any C file that includes
the header and doesn't use the function will emit a "defined but not
used warning".
Functions in headers also should, by definition, be small and
specifically inline-able (ie they are used as a type-safe macro).
static functions in C files (not headers) should not have the inline
keyword as GCC can optimize them and inline them as it sees fit and the
inline keyword doesn't actually do anything.
Logan
On 4/28/22 11:44 PM, Logan Gunthorpe wrote:
>
>
> On 2022-04-27 19:49, Guoqing Jiang wrote:
>>
>> On 4/28/22 12:07 AM, Logan Gunthorpe wrote:
>>> On 2022-04-26 19:28, Guoqing Jiang wrote:
>>>>> +static bool ahead_of_reshape(struct mddev *mddev, sector_t sector,
>>>>> + sector_t reshape_sector)
>>>>> +{
>>>>> + if (mddev->reshape_backwards)
>>>>> + return sector < reshape_sector;
>>>>> + else
>>>>> + return sector >= reshape_sector;
>>>>> +}
>>>> I think it can be an inline function.
>>> Marking static functions in C files as inline is not recommended. GCC
>>> will inline it, if it is appropriate.
>>>
>>> https://yarchive.net/comp/linux/inline.html
>>> https://www.kernel.org/doc/local/inline.html
>> Thanks for the link, then I suppose those can be deleted
>> linux> grep "static inline" drivers/md/md.h -r
> It's standard practice to annotate small functions in headers with
> "static inline". Without the inline annotation, any C file that includes
> the header and doesn't use the function will emit a "defined but not
> used warning".
>
> Functions in headers also should, by definition, be small and
> specifically inline-able (ie they are used as a type-safe macro).
>
> static functions in C files (not headers) should not have the inline
> keyword as GCC can optimize them and inline them as it sees fit and the
> inline keyword doesn't actually do anything.
I am happy to be taught, but still I can see lots of static function in
C files
as well, at least
linux> grep "static inline" drivers/md/*.c -r|wc
98 661 8630
Anyway, I don't want to argue about it anymore.
Thanks,
Guoqing