Re: [PATCH 1/7] block: add support for carrying a stream ID in a bio

From: Andreas Dilger <adilger@dilger.ca>
To: Jens Axboe <axboe@fb.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	ming.l@ssi.samsung.com, david@fromorbit.com
Subject: Re: [PATCH 1/7] block: add support for carrying a stream ID in a bio
Date: Thu, 9 Apr 2015 16:46:29 -0600	[thread overview]
Message-ID: <E227B6B0-F6D8-468A-80E9-7DEB3AF833EE@dilger.ca> (raw)
In-Reply-To: <1427296070-8472-2-git-send-email-axboe@fb.com>

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