| Message ID | 1427296070-8472-2-git-send-email-axboe@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mar 25, 2015, at 9:07 AM, Jens Axboe <axboe@fb.com> wrote: > > The top bits of bio->bi_flags are reserved for keeping the > allocation pool, set aside the next eight bits for carrying > a stream ID. That leaves us with support for 255 streams, > 0 is reserved as a "stream not set" value. I understand that the stream ID is not related to specific priority of an IO request. However, I'm wondering how this patch series interacts with some of the other patch series that add cache priority hints? Is there a danger of running out of space in the IO pipeline for the additional cache hints if this is using 8 bits? Some more comments inline. > Add helpers for setting/getting stream ID of a bio. > > Signed-off-by: Jens Axboe <axboe@fb.com> > --- > block/bio.c | 2 ++ > block/blk-core.c | 3 +++ > include/linux/blk_types.h | 28 +++++++++++++++++++++++++++- > 3 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index f66a4eae16ee..1cd3d745047c 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -567,6 +567,7 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) > bio->bi_rw = bio_src->bi_rw; > bio->bi_iter = bio_src->bi_iter; > bio->bi_io_vec = bio_src->bi_io_vec; > + bio_set_streamid(bio, bio_get_streamid(bio_src)); > } > EXPORT_SYMBOL(__bio_clone_fast); > > @@ -672,6 +673,7 @@ integrity_clone: > } > } > > + bio_set_streamid(bio, bio_get_streamid(bio_src)); > return bio; > } > EXPORT_SYMBOL(bio_clone_bioset); > diff --git a/block/blk-core.c b/block/blk-core.c > index 794c3e7f01cf..6b7b8c95c4c3 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -1928,6 +1928,9 @@ void generic_make_request(struct bio *bio) > do { > struct request_queue *q = bdev_get_queue(bio->bi_bdev); > > + if (bio_streamid_valid(bio)) > + blk_add_trace_msg(q, "StreamID=%u", bio_get_streamid(bio)); > + > q->make_request_fn(q, bio); > > bio = bio_list_pop(current->bio_list); > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h > index c294e3e25e37..d6909007a3fe 100644 > --- a/include/linux/blk_types.h > +++ b/include/linux/blk_types.h > @@ -138,9 +138,35 @@ struct bio { > #define BIO_POOL_BITS (4) > #define BIO_POOL_NONE ((1UL << BIO_POOL_BITS) - 1) > #define BIO_POOL_OFFSET (BITS_PER_LONG - BIO_POOL_BITS) > -#define BIO_POOL_MASK (1UL << BIO_POOL_OFFSET) > #define BIO_POOL_IDX(bio) ((bio)->bi_flags >> BIO_POOL_OFFSET) > > +/* > + * after the pool bits, next 8 bits are for the stream id > + */ > +#define BIO_STREAM_BITS (8) > +#define BIO_STREAM_OFFSET (BITS_PER_LONG - 12) Should this really be: #define BIO_STREAM_OFFSET (BIO_POOL_OFFSET - BIO_STREAM_BITS) Otherwise there is a risk of conflict if someone changes BIO_POOL_BITS. Cheers, Andreas > +#define BIO_STREAM_MASK ((1 << BIO_STREAM_BITS) - 1) > + > +static inline unsigned long streamid_to_flags(unsigned int id) > +{ > + return (unsigned long) (id & BIO_STREAM_MASK) << BIO_STREAM_OFFSET; > +} > + > +static inline void bio_set_streamid(struct bio *bio, unsigned int id) > +{ > + bio->bi_flags |= streamid_to_flags(id); > +} > + > +static inline unsigned int bio_get_streamid(struct bio *bio) > +{ > + return (bio->bi_flags >> BIO_STREAM_OFFSET) & BIO_STREAM_MASK; > +} > + > +static inline bool bio_streamid_valid(struct bio *bio) > +{ > + return bio_get_streamid(bio) != 0; > +} > + > #endif /* CONFIG_BLOCK */ > > /* > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/09/2015 04:46 PM, Andreas Dilger wrote: > On Mar 25, 2015, at 9:07 AM, Jens Axboe <axboe@fb.com> wrote: >> >> The top bits of bio->bi_flags are reserved for keeping the >> allocation pool, set aside the next eight bits for carrying >> a stream ID. That leaves us with support for 255 streams, >> 0 is reserved as a "stream not set" value. > > I understand that the stream ID is not related to specific priority > of an IO request. However, I'm wondering how this patch series > interacts with some of the other patch series that add cache priority > hints? Is there a danger of running out of space in the IO pipeline > for the additional cache hints if this is using 8 bits? That's always a risk, of course, but that goes for most features that need to carry more data in struct bio (or elsewhere). Otherwise we'll have to bite the bullet and add a new field. >> +/* >> + * after the pool bits, next 8 bits are for the stream id >> + */ >> +#define BIO_STREAM_BITS (8) >> +#define BIO_STREAM_OFFSET (BITS_PER_LONG - 12) > > Should this really be: > > #define BIO_STREAM_OFFSET (BIO_POOL_OFFSET - BIO_STREAM_BITS) > > Otherwise there is a risk of conflict if someone changes BIO_POOL_BITS. Good point, that would be cleaner. I'll make that change.
diff --git a/block/bio.c b/block/bio.c index f66a4eae16ee..1cd3d745047c 100644 --- a/block/bio.c +++ b/block/bio.c @@ -567,6 +567,7 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) bio->bi_rw = bio_src->bi_rw; bio->bi_iter = bio_src->bi_iter; bio->bi_io_vec = bio_src->bi_io_vec; + bio_set_streamid(bio, bio_get_streamid(bio_src)); } EXPORT_SYMBOL(__bio_clone_fast); @@ -672,6 +673,7 @@ integrity_clone: } } + bio_set_streamid(bio, bio_get_streamid(bio_src)); return bio; } EXPORT_SYMBOL(bio_clone_bioset); diff --git a/block/blk-core.c b/block/blk-core.c index 794c3e7f01cf..6b7b8c95c4c3 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1928,6 +1928,9 @@ void generic_make_request(struct bio *bio) do { struct request_queue *q = bdev_get_queue(bio->bi_bdev); + if (bio_streamid_valid(bio)) + blk_add_trace_msg(q, "StreamID=%u", bio_get_streamid(bio)); + q->make_request_fn(q, bio); bio = bio_list_pop(current->bio_list); diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index c294e3e25e37..d6909007a3fe 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -138,9 +138,35 @@ struct bio { #define BIO_POOL_BITS (4) #define BIO_POOL_NONE ((1UL << BIO_POOL_BITS) - 1) #define BIO_POOL_OFFSET (BITS_PER_LONG - BIO_POOL_BITS) -#define BIO_POOL_MASK (1UL << BIO_POOL_OFFSET) #define BIO_POOL_IDX(bio) ((bio)->bi_flags >> BIO_POOL_OFFSET) +/* + * after the pool bits, next 8 bits are for the stream id + */ +#define BIO_STREAM_BITS (8) +#define BIO_STREAM_OFFSET (BITS_PER_LONG - 12) +#define BIO_STREAM_MASK ((1 << BIO_STREAM_BITS) - 1) + +static inline unsigned long streamid_to_flags(unsigned int id) +{ + return (unsigned long) (id & BIO_STREAM_MASK) << BIO_STREAM_OFFSET; +} + +static inline void bio_set_streamid(struct bio *bio, unsigned int id) +{ + bio->bi_flags |= streamid_to_flags(id); +} + +static inline unsigned int bio_get_streamid(struct bio *bio) +{ + return (bio->bi_flags >> BIO_STREAM_OFFSET) & BIO_STREAM_MASK; +} + +static inline bool bio_streamid_valid(struct bio *bio) +{ + return bio_get_streamid(bio) != 0; +} + #endif /* CONFIG_BLOCK */ /*
The top bits of bio->bi_flags are reserved for keeping the allocation pool, set aside the next eight bits for carrying a stream ID. That leaves us with support for 255 streams, 0 is reserved as a "stream not set" value. Add helpers for setting/getting stream ID of a bio. Signed-off-by: Jens Axboe <axboe@fb.com> --- block/bio.c | 2 ++ block/blk-core.c | 3 +++ include/linux/blk_types.h | 28 +++++++++++++++++++++++++++- 3 files changed, 32 insertions(+), 1 deletion(-)