md: bitwise operations might not fit in a "bool"
in [Kernel]
|
Prev: [tip:perf/urgent] tracing: Properly align linker defined symbols
Next: [PATCH] CIFS: Fix a malicious redirect problem in the DNS lookup code
From: Neil Brown on 22 Jul 2010 08:00 On Thu, 22 Jul 2010 14:44:53 +0300 Boaz Harrosh <bharrosh(a)panasas.com> wrote: > > when taking a resolute of a bit-wise AND as true false. Better / faster > to make it a boolean operation. > > This fixes a bug and a crash because the flags field did not fit into > the bool operands. No, that won't work. Read the rest of the code and see where 'do_sync' and 'do_barriers' are used. NeilBrown > > Signed-off-by: Boaz Harrosh <bharrosh(a)panasas.com> > --- > git diff --stat -p -M drivers/md/raid1.c > drivers/md/raid1.c | 11 +++++++---- > 1 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index 73cc74f..67a9159 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -787,7 +787,7 @@ static int make_request(mddev_t *mddev, struct bio * bio) > struct bio_list bl; > struct page **behind_pages = NULL; > const int rw = bio_data_dir(bio); > - const bool do_sync = (bio->bi_rw & REQ_SYNC); > + const bool do_sync = (bio->bi_rw & REQ_SYNC) != 0; > bool do_barriers; > mdk_rdev_t *blocked_rdev; > > @@ -959,7 +959,7 @@ static int make_request(mddev_t *mddev, struct bio * bio) > atomic_set(&r1_bio->remaining, 0); > atomic_set(&r1_bio->behind_remaining, 0); > > - do_barriers = bio->bi_rw & REQ_HARDBARRIER; > + do_barriers = (bio->bi_rw & REQ_HARDBARRIER) != 0; > if (do_barriers) > set_bit(R1BIO_Barrier, &r1_bio->state); > > @@ -1640,7 +1640,8 @@ static void raid1d(mddev_t *mddev) > * We already have a nr_pending reference on these rdevs. > */ > int i; > - const bool do_sync = (r1_bio->master_bio->bi_rw & REQ_SYNC); > + const bool do_sync = > + (r1_bio->master_bio->bi_rw & REQ_SYNC) != 0; > clear_bit(R1BIO_BarrierRetry, &r1_bio->state); > clear_bit(R1BIO_Barrier, &r1_bio->state); > for (i=0; i < conf->raid_disks; i++) > @@ -1696,7 +1697,9 @@ static void raid1d(mddev_t *mddev) > (unsigned long long)r1_bio->sector); > raid_end_bio_io(r1_bio); > } else { > - const bool do_sync = r1_bio->master_bio->bi_rw & REQ_SYNC; > + const bool do_sync = > + (r1_bio->master_bio->bi_rw & REQ_SYNC) > + != 0; > r1_bio->bios[r1_bio->read_disk] = > mddev->ro ? IO_BLOCKED : NULL; > r1_bio->read_disk = disk; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Boaz Harrosh on 22 Jul 2010 08:30 On 07/22/2010 02:55 PM, Neil Brown wrote: > On Thu, 22 Jul 2010 14:44:53 +0300 > Boaz Harrosh <bharrosh(a)panasas.com> wrote: > >> >> when taking a resolute of a bit-wise AND as true false. Better / faster >> to make it a boolean operation. >> >> This fixes a bug and a crash because the flags field did not fit into >> the bool operands. > > No, that won't work. > Read the rest of the code and see where 'do_sync' and 'do_barriers' are used. > > NeilBrown > You are right! (I didn't look) the use of "bool" was wrong from the get go. it was never a bool operation. What was the guy thinking? What is that do_XXX name? that name should change as well. Perhaps flg_sync, flg_barriers. Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Neil Brown on 22 Jul 2010 08:40 On Thu, 22 Jul 2010 15:20:53 +0300 Boaz Harrosh <bharrosh(a)panasas.com> wrote: > On 07/22/2010 02:55 PM, Neil Brown wrote: > > On Thu, 22 Jul 2010 14:44:53 +0300 > > Boaz Harrosh <bharrosh(a)panasas.com> wrote: > > > >> > >> when taking a resolute of a bit-wise AND as true false. Better / faster > >> to make it a boolean operation. > >> > >> This fixes a bug and a crash because the flags field did not fit into > >> the bool operands. > > > > No, that won't work. > > Read the rest of the code and see where 'do_sync' and 'do_barriers' are used. > > > > NeilBrown > > > > You are right! (I didn't look) > > the use of "bool" was wrong from the get go. it was never a bool operation. > What was the guy thinking? What is that do_XXX name? that name should change > as well. Perhaps flg_sync, flg_barriers. Check the git history - 'bool' was originally appropriate. But when the value was recently changed, the type and name were not. I would actually prefer "sync_flg" and "barrier_flg", but your suggestion that we change the name as well as the type is a good one. Thanks, NeilBrown -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 27 Jul 2010 18:30
>> - const bool do_sync = (bio->bi_rw & REQ_SYNC); >> + const bool do_sync = (bio->bi_rw & REQ_SYNC) != 0; FWIW, this is a null change. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |