[PATCH] Fix setting bio flags in drivers (sd_dif/floppy).

[Muthu Kumar]

Fix setting bio flags in drivers (sd_dif/floppy).

Signed-off-by: Muthukumar R <mut...@gmail.com>

---------------------------

drivers/block/floppy.c | 2 +-
drivers/scsi/sd_dif.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

------------------------

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 9baf11e..744f078 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3832,7 +3832,7 @@ static int __floppy_read_block_0(struct
block_device *bdev)
bio.bi_size = size;
bio.bi_bdev = bdev;
bio.bi_sector = 0;
- bio.bi_flags = BIO_QUIET;
+ bio.bi_flags = (1 << BIO_QUIET);
init_completion(&complete);
bio.bi_private = &complete;
bio.bi_end_io = floppy_rb0_complete;
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 0cb39ff..f8fb2d6 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -408,7 +408,7 @@ int sd_dif_prepare(struct request *rq, sector_t
hw_sector, unsigned int sector_s
kunmap_atomic(sdt, KM_USER0);
}

- bio->bi_flags |= BIO_MAPPED_INTEGRITY;
+ bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
}

return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Linus Torvalds]

On Wed, Feb 29, 2012 at 3:22 PM, Muthu Kumar <muthu...@gmail.com> wrote:
> Fix setting bio flags in drivers (sd_dif/floppy).

A quick grep seems to agree that these are the only obvious ones.

Jens? Will you send this to me, or should I take it directly?

And when did Andrew change his name? Nobody tells me these things.

Linus

[Andrew Morton]

On Wed, 29 Feb 2012 18:22:14 -0500
Muthu Kumar <muthu...@gmail.com> wrote:

> Fix setting bio flags in drivers (sd_dif/floppy).
>

> ...

>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 9baf11e..744f078 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3832,7 +3832,7 @@ static int __floppy_read_block_0(struct
> block_device *bdev)
> bio.bi_size = size;
> bio.bi_bdev = bdev;
> bio.bi_sector = 0;
> - bio.bi_flags = BIO_QUIET;
> + bio.bi_flags = (1 << BIO_QUIET);
> init_completion(&complete);
> bio.bi_private = &complete;
> bio.bi_end_io = floppy_rb0_complete;
> diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
> index 0cb39ff..f8fb2d6 100644
> --- a/drivers/scsi/sd_dif.c
> +++ b/drivers/scsi/sd_dif.c
> @@ -408,7 +408,7 @@ int sd_dif_prepare(struct request *rq, sector_t
> hw_sector, unsigned int sector_s
> kunmap_atomic(sdt, KM_USER0);
> }
>
> - bio->bi_flags |= BIO_MAPPED_INTEGRITY;
> + bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
> }
>
> return 0;

urgh. This isn't the first time.

It's too easy for people to make this mistake. I'm not sure what a
good fix would be - I don't think sparse can save us with __bitwise or
similar.

The approach we took in buffer_head.h with BH_Foo and BUFFER_FNS
accessors worked pretty well.

[Andrew Morton]

On Wed, 29 Feb 2012 16:08:15 -0800
Linus Torvalds <torv...@linux-foundation.org> wrote:

> On Wed, Feb 29, 2012 at 3:22 PM, Muthu Kumar <muthu...@gmail.com> wrote:
> > Fix setting bio flags in drivers (sd_dif/floppy).
>
> A quick grep seems to agree that these are the only obvious ones.
>
> Jens? Will you send this to me, or should I take it directly?

I'll send at you tomorrowish. Or you can take it directly, after fixing
the wordwrapping ;)

I don't know if a -stable backport is warranted...

> And when did Andrew change his name? Nobody tells me these things.

oh gawd no. Somehow google managed to replace my name with my son's in
various people's address books. Now it's gone and done it with
ak...@linux-foundation.org as well as ak...@google.com. I'm in a losing
fight trying to prevent those two identities from getting intermingled,
absorbed and generally borgified. And how did google know that we're
related? I can only think that it snarfed that connection from
facebook.

[Martin K. Petersen]

>>>>> "Andrew" == Andrew Morton <ak...@linux-foundation.org> writes:

>> + bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
>> }
>>
>> return 0;

Andrew> urgh. This isn't the first time.

Andrew> It's too easy for people to make this mistake. I'm not sure
Andrew> what a good fix would be - I don't think sparse can save us with
Andrew> __bitwise or similar.

Yeah. I actually got bitten by this recently.

This is what I'm doing in my current working tree (this particular flag
has moved for other reasons). But we could provide a similar wrapper for
bio->bi_flags...

[...]
+static inline unsigned int bio_integrity_flagged(struct bio *bio, enum bip_flags flag)
+{
+ if (bio && bio->bi_integrity)
+ return bio->bi_integrity->bip_flags & (1 << flag);
+
+ return 0;
+}
+
+static inline void bio_integrity_flag(struct bio *bio, enum bip_flags flag)
+{
+ if (bio && bio->bi_integrity)
+ bio->bi_integrity->bip_flags |= (1 << flag);
+}
[...]

--
Martin K. Petersen Oracle Linux Engineering

[Jens Axboe]

On 03/01/2012 01:08 AM, Linus Torvalds wrote:
> On Wed, Feb 29, 2012 at 3:22 PM, Muthu Kumar <muthu...@gmail.com> wrote:
>> Fix setting bio flags in drivers (sd_dif/floppy).
>
> A quick grep seems to agree that these are the only obvious ones.
>
> Jens? Will you send this to me, or should I take it directly?

Which ever one comes first. I've got a few other items queued up that I
will send shortly.

--
Jens Axboe

[Jens Axboe]

On 2012-03-01 21:23, Muthu Kumar wrote:
>>> kunmap_atomic(sdt, KM_USER0);
>>> }
>>>
>>> - bio->bi_flags |= BIO_MAPPED_INTEGRITY;
>>> + bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
>>> }
>>>
>>> return 0;
>>
>> urgh. This isn't the first time.
>>
>> It's too easy for people to make this mistake. I'm not sure what a
>> good fix would be - I don't think sparse can save us with __bitwise or
>> similar.
>>
>> The approach we took in buffer_head.h with BH_Foo and BUFFER_FNS
>> accessors worked pretty well.
>>
>

> Does this look good? BTW, I used the non-atomic variants __set/__clear
> to match the behavior of current usage.
> If this looks good, I can send the proper patch as attachment (so no
> line wraps :)

Lets not wrap this in macros, it just makes the code harder to read.
Making the BIO_ flags a bit shift value was a mistake in hindsight, too
easy to get wrong.

If we're going to be changing everything anyway, it'd be better to have
__ variants if we really need the bit shifts, and the non __ variants be
the value. Similar to what is done for request flags.

--
Jens Axboe

[Muthu Kumar]

>>                       kunmap_atomic(sdt, KM_USER0);
>>               }
>>
>> -             bio->bi_flags |= BIO_MAPPED_INTEGRITY;
>> +             bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
>>       }
>>
>>       return 0;
>
> urgh.  This isn't the first time.
>
> It's too easy for people to make this mistake.  I'm not sure what a
> good fix would be - I don't think sparse can save us with __bitwise or
> similar.
>
> The approach we took in buffer_head.h with BH_Foo and BUFFER_FNS
> accessors worked pretty well.
>

Does this look good? BTW, I used the non-atomic variants __set/__clear
to match the behavior of current usage.
If this looks good, I can send the proper patch as attachment (so no
line wraps :)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4053cbd..8b2fbbb 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -8,6 +8,7 @@
#ifdef CONFIG_BLOCK

#include <linux/types.h>
+#include <linux/bitops.h>

struct bio_set;
struct bio;
@@ -98,6 +99,43 @@ struct bio {
#define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))

/*
+ * Macro tricks similar to setting b_state in buffer_head.h.
+ * To set uptodate flag in a bio:
+ * bio_flags_set_uptodate(bio);
+ * To clear
+ * bio_flags_clear_uptodate(bio);
+ * To test if it is set
+ * bio_flagged_uptodate(bio);
+ */
+#define BIO_FLAGS_FNS(bit, name) \
+static inline void bio_flags_set_##name(struct bio *bio) \
+{ \
+ __set_bit(BIO_##bit, &(bio)->bi_flags); \
+} \
+static inline void bio_flags_clear_##name(struct bio *bio) \
+{ \
+ __clear_bit(BIO_##bit, &(bio)->bi_flags); \
+} \
+static inline int bio_flagged_##name(const struct bio *bio) \
+{ \
+ return test_bit(BIO_##bit, &(bio)->bi_flags); \
+} \
+
+BIO_FLAGS_FNS(UPTODATE, uptodate)
+BIO_FLAGS_FNS(RW_BLOCK, rw_block)
+BIO_FLAGS_FNS(EOF, eof)
+BIO_FLAGS_FNS(SEG_VALID, seg_valid)
+BIO_FLAGS_FNS(CLONED, cloned)
+BIO_FLAGS_FNS(BOUNCED, bounced)
+BIO_FLAGS_FNS(USER_MAPPED, user_mapped)
+BIO_FLAGS_FNS(EOPNOTSUPP, eopnotsupp)
+BIO_FLAGS_FNS(NULL_MAPPED, null_mapped)
+BIO_FLAGS_FNS(FS_INTEGRITY, fs_integrity)
+BIO_FLAGS_FNS(QUIET, quiet)
+BIO_FLAGS_FNS(MAPPED_INTEGRITY, mapped_integrity)
+
+/*
* top 4 bits of bio flags indicate the pool this bio came from
*/
#define BIO_POOL_BITS (4)

[Muthu Kumar]

On Thu, Mar 1, 2012 at 12:25 PM, Jens Axboe <ax...@kernel.dk> wrote:
> On 2012-03-01 21:23, Muthu Kumar wrote:
>>>>                       kunmap_atomic(sdt, KM_USER0);
>>>>               }
>>>>
>>>> -             bio->bi_flags |= BIO_MAPPED_INTEGRITY;
>>>> +             bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
>>>>       }
>>>>
>>>>       return 0;
>>>
>>> urgh.  This isn't the first time.
>>>
>>> It's too easy for people to make this mistake.  I'm not sure what a
>>> good fix would be - I don't think sparse can save us with __bitwise or
>>> similar.
>>>
>>> The approach we took in buffer_head.h with BH_Foo and BUFFER_FNS
>>> accessors worked pretty well.
>>>
>>
>> Does this look good? BTW, I used the non-atomic variants __set/__clear
>> to match the behavior of current usage.
>> If this looks good, I can send the proper patch as attachment (so no
>> line wraps :)
>
> Lets not wrap this in macros, it just makes the code harder to read.
> Making the BIO_ flags a bit shift value was a mistake in hindsight, too
> easy to get wrong.
>
> If we're going to be changing everything anyway, it'd be better to have
> __ variants if we really need the bit shifts, and the non __ variants be
> the value. Similar to what is done for request flags.
>
> --
> Jens Axboe
>

You mean, like this? (sparing the gmail line wrap)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4053cbd..035e1ef 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -83,19 +83,35 @@ struct bio {
/*
* bio flags
*/
-#define BIO_UPTODATE 0 /* ok after I/O completion */
-#define BIO_RW_BLOCK 1 /* RW_AHEAD set, and read/write would block */
-#define BIO_EOF 2 /* out-out-bounds error */
-#define BIO_SEG_VALID 3 /* bi_phys_segments valid */
-#define BIO_CLONED 4 /* doesn't own data */
-#define BIO_BOUNCED 5 /* bio is a bounce bio */
-#define BIO_USER_MAPPED 6 /* contains user pages */
-#define BIO_EOPNOTSUPP 7 /* not supported */
-#define BIO_NULL_MAPPED 8 /* contains invalid user pages */
-#define BIO_FS_INTEGRITY 9 /* fs owns integrity data, not block layer */
-#define BIO_QUIET 10 /* Make BIO Quiet */
-#define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
-#define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))
+enum bio_flag_bits {
+ __BIO_UPTODATE, /* ok after I/O completion */
+ __BIO_RW_BLOCK, /* RW_AHEAD set, and read/write would block */
+ __BIO_EOF, /* out-out-bounds error */
+ __BIO_SEG_VALID, /* bi_phys_segments valid */
+ __BIO_CLONED, /* doesn't own data */
+ __BIO_BOUNCED, /* bio is a bounce bio */
+ __BIO_USER_MAPPED, /* contains user pages */
+ __BIO_EOPNOTSUPP, /* not supported */
+ __BIO_NULL_MAPPED, /* contains invalid user pages */
+ __BIO_FS_INTEGRITY, /* fs owns integrity data, not block layer */
+ __BIO_QUIET, /* Make BIO Quiet */
+ __BIO_MAPPED_INTEGRITY, /* integrity metadata has been remapped */
+};
+
+#define BIO_UPTODATE (1 << __BIO_UPTODATE)
+#define BIO_RW_BLOCK (1 << __BIO_RW_BLOCK)
+#define BIO_EOF (1 << __BIO_EOF)
+#define BIO_SEG_VALID (1 << __BIO_SEG_VALID)
+#define BIO_CLONED (1 << __BIO_CLONED)
+#define BIO_BOUNCED (1 << __BIO_BOUNCED)
+#define BIO_USER_MAPPED (1 << __BIO_USER_MAPPED)
+#define BIO_EOPNOTSUPP (1 << __BIO_EOPNOTSUPP)
+#define BIO_NULL_MAPPED (1 << __BIO_NULL_MAPPED)
+#define BIO_FS_INTEGRITY (1 << __BIO_FS_INTEGRITY)
+#define BIO_QUIET (1 << __BIO_QUIET)
+#define BIO_MAPPED_INTEGRITY (1 << __BIO_MAPPED_INTEGRITY)
+
+#define bio_flagged(bio, flag) ((bio)->bi_flags & (flag))

/*
* top 4 bits of bio flags indicate the pool this bio came from

[Jens Axboe]

Yes, exactly. Only problem is that it would be a big patch, and old code
would still compile. We'd probably need to make all those a bit
different, ala

+#define BIO_F_UPTODATE (1 << __BIO_UPTODATE)

to be completely safe and catch any use case.

--
Jens Axboe