| Message ID | 1553846032-4451-4-git-send-email-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extend write-hint for in-kernel use | expand |
On Fri, Mar 29, 2019 at 01:23:48PM +0530, Kanchan Joshi wrote: > Earlier this conversion was done by driver (nvme). Current conversion > is of the form "streamid = write-hint - 1", for both user and kernel > streams (note that existing infra takes care that user-streams do not > bump into kernel ones). Unless we add new user streams, then all the kernel streams change ID. I'll deal with this in more detail in a later patch. > Conversion takes stream limit (maintained in > request queue) into account. Write-hints beyond the queue-limit turn > to 0. > New field 'streamid' has been added in request. While 'write-hint' field > continues to exist. It keeps original value passed from upper layer, and > used during merging checks. > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > --- > block/blk-core.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 3c5f61c..c86daed 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -727,6 +727,25 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, > return false; > } > > +enum rw_hint blk_write_hint_to_streamid(struct request *req, > + struct bio *bio) > +{ > + enum rw_hint streamid, nr_streams; > + struct request_queue *q = req->q; > + nr_streams = q->limits.nr_streams; > + > + streamid = bio->bi_write_hint; > + if (!nr_streams || streamid == WRITE_LIFE_NOT_SET || > + streamid == WRITE_LIFE_NONE) > + streamid = 0; > + else { > + --streamid; What's this magic thing do? > + if(streamid > nr_streams) > + streamid = 0; So, basically, we'll compress all the kernel hints down to "no hint" if there are more user streams than the device supports? Surely we should be reserving a stream for the kernel hints separate from the user and "none" streams when we have limited device streams available... Cheers, Dave.
On Mon 01-04-19 16:08:21, Dave Chinner wrote: > On Fri, Mar 29, 2019 at 01:23:48PM +0530, Kanchan Joshi wrote: > > + if(streamid > nr_streams) > > + streamid = 0; > > So, basically, we'll compress all the kernel hints down to "no hint" > if there are more user streams than the device supports? > > Surely we should be reserving a stream for the kernel hints separate > from the user and "none" streams when we have limited device streams > available... The question is what to do in a situation when the device has exactly as many hints as we currently offer to userspace. Because currently either the device has enough hints for all userspace hint values or we disable the feature altogether. If we always mandated some hints are available for the kernel, we'd have to regress some fuctionality currently available to userspace. So I think that the option that the kernel won't get any hints is the least painful solution. Later when people would like to extend hints available to userspace, we could make sure kernel's batch of hints has priority over these "extended userspace hints". Honza
On Tue, Apr 02, 2019 at 11:20:44AM +0200, Jan Kara wrote: > On Mon 01-04-19 16:08:21, Dave Chinner wrote: > > On Fri, Mar 29, 2019 at 01:23:48PM +0530, Kanchan Joshi wrote: > > > + if(streamid > nr_streams) > > > + streamid = 0; > > > > So, basically, we'll compress all the kernel hints down to "no hint" > > if there are more user streams than the device supports? > > > > Surely we should be reserving a stream for the kernel hints separate > > from the user and "none" streams when we have limited device streams > > available... > > The question is what to do in a situation when the device has exactly as > many hints as we currently offer to userspace. Then do what we do now for that case. For every other case, the kernel should have reserved space and not get intermingled with userspace hints. Cheers, Dave.
On Wed 03-04-19 07:35:08, Dave Chinner wrote: > On Tue, Apr 02, 2019 at 11:20:44AM +0200, Jan Kara wrote: > > On Mon 01-04-19 16:08:21, Dave Chinner wrote: > > > On Fri, Mar 29, 2019 at 01:23:48PM +0530, Kanchan Joshi wrote: > > > > + if(streamid > nr_streams) > > > > + streamid = 0; > > > > > > So, basically, we'll compress all the kernel hints down to "no hint" > > > if there are more user streams than the device supports? > > > > > > Surely we should be reserving a stream for the kernel hints separate > > > from the user and "none" streams when we have limited device streams > > > available... > > > > The question is what to do in a situation when the device has exactly as > > many hints as we currently offer to userspace. > > Then do what we do now for that case. For every other case, the > kernel should have reserved space and not get intermingled with > userspace hints. Yup, we are on the same page then. Honza
> Then do what we do now for that case. For every other case, the kernel > should have reserved space and not get intermingled with userspace > hints. I hope this means that we're fine with the current conversion approach. As you would have noticed, current approach does not disable stream feature based on dearth of streams. It either exposes 8 streams (if device has equal or more than 8 streams) or N streams (if N is less than 8). For less than 8 streams case, user-space hints get priority over kernel-space hints. But at any point of time, there is no intermingling. Thanks, Kanchan -----Original Message----- From: Jan Kara [mailto:jack@suse.cz] Sent: Wednesday, April 03, 2019 3:06 PM To: Dave Chinner <david@fromorbit.com> Cc: Jan Kara <jack@suse.cz>; Kanchan Joshi <joshi.k@samsung.com>; linux-kernel@vger.kernel.org; linux-block@vger.kernel.org; linux-nvme@lists.infradead.org; linux-fsdevel@vger.kernel.org; linux-ext4@vger.kernel.org; axboe@fb.com; prakash.v@samsung.com; anshul@samsung.com; joshiiitr@gmail.com Subject: Re: [PATCH v3 3/7] block: add write-hint to stream-id conversion On Wed 03-04-19 07:35:08, Dave Chinner wrote: > On Tue, Apr 02, 2019 at 11:20:44AM +0200, Jan Kara wrote: > > On Mon 01-04-19 16:08:21, Dave Chinner wrote: > > > On Fri, Mar 29, 2019 at 01:23:48PM +0530, Kanchan Joshi wrote: > > > > + if(streamid > nr_streams) > > > > + streamid = 0; > > > > > > So, basically, we'll compress all the kernel hints down to "no hint" > > > if there are more user streams than the device supports? > > > > > > Surely we should be reserving a stream for the kernel hints > > > separate from the user and "none" streams when we have limited > > > device streams available... > > > > The question is what to do in a situation when the device has > > exactly as many hints as we currently offer to userspace. > > Then do what we do now for that case. For every other case, the kernel > should have reserved space and not get intermingled with userspace > hints. Yup, we are on the same page then. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
diff --git a/block/blk-core.c b/block/blk-core.c index 3c5f61c..c86daed 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -727,6 +727,25 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, return false; } +enum rw_hint blk_write_hint_to_streamid(struct request *req, + struct bio *bio) +{ + enum rw_hint streamid, nr_streams; + struct request_queue *q = req->q; + nr_streams = q->limits.nr_streams; + + streamid = bio->bi_write_hint; + if (!nr_streams || streamid == WRITE_LIFE_NOT_SET || + streamid == WRITE_LIFE_NONE) + streamid = 0; + else { + --streamid; + if(streamid > nr_streams) + streamid = 0; + } + return streamid; +} + void blk_init_request_from_bio(struct request *req, struct bio *bio) { if (bio->bi_opf & REQ_RAHEAD) @@ -735,6 +754,7 @@ void blk_init_request_from_bio(struct request *req, struct bio *bio) req->__sector = bio->bi_iter.bi_sector; req->ioprio = bio_prio(bio); req->write_hint = bio->bi_write_hint; + req->streamid = blk_write_hint_to_streamid(req, bio); blk_rq_bio_prep(req->q, req, bio); } EXPORT_SYMBOL_GPL(blk_init_request_from_bio);
Earlier this conversion was done by driver (nvme). Current conversion is of the form "streamid = write-hint - 1", for both user and kernel streams (note that existing infra takes care that user-streams do not bump into kernel ones). Conversion takes stream limit (maintained in request queue) into account. Write-hints beyond the queue-limit turn to 0. New field 'streamid' has been added in request. While 'write-hint' field continues to exist. It keeps original value passed from upper layer, and used during merging checks. Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> --- block/blk-core.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)