> Someone told me that stream ids are potentially persistent on the > storage so it isn't great to change the id for the same thing e.g. if > we add another user hint. So maybe we should allocate kernel hints > from top as Dave Chinner suggested? I.e., something like the following mapping function: This function is good. Thank you for sharing. -----Original Message----- From: Andreas Dilger [mailto:adilger@xxxxxxxxx] Sent: Friday, April 19, 2019 12:28 AM To: Jan Kara <jack@xxxxxxx> Cc: Kanchan Joshi <joshi.k@xxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>; linux-block <linux-block@xxxxxxxxxxxxxxx>; linux-nvme@xxxxxxxxxxxxxxxxxxx; linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>; linux-ext4@xxxxxxxxxxxxxxx; prakash.v@xxxxxxxxxxx Subject: Re: [PATCH v4 4/7] block: introduce write-hint to stream-id conversion On Apr 18, 2019, at 8:06 AM, Jan Kara <jack@xxxxxxx> wrote: > > On Wed 17-04-19 23:20:03, Kanchan Joshi wrote: >> This patch moves write-hint-to-stream-id conversion in block-layer. >> Earlier this was done by driver (nvme). Current conversion is of the >> form "streamid = write-hint - 1", for both user and kernel streams. >> Conversion takes stream limit (maintained in request queue) into >> account. Write-hints beyond the exposed limit turn to 0. >> A 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@xxxxxxxxxxx> >> --- >> block/blk-core.c | 20 ++++++++++++++++++++ >> include/linux/blkdev.h | 1 + >> 2 files changed, 21 insertions(+) >> >> diff --git a/block/blk-core.c b/block/blk-core.c index >> a55389b..712e6b7 100644 >> --- a/block/blk-core.c >> +++ b/block/blk-core.c >> @@ -730,6 +730,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; >> +} >> + > > Someone told me that stream ids are potentially persistent on the > storage so it isn't great to change the id for the same thing e.g. if > we add another user hint. So maybe we should allocate kernel hints > from top as Dave Chinner suggested? I.e., something like the following mapping function: > > if (streamid <= BLK_MAX_USER_HINTS) { > streamid--; > if (streamid > nr_streams) > streamid = 0; > } else { > /* Kernel hints get allocated from top */ > streamid -= WRITE_LIFE_KERN_MIN; > if (streamid > nr_streams - BLK_MAX_USER_HINTS) > streamid = 0; > else > streamid = nr_streams - streamid - 1; > } > > what do you think? Dave has expressed this sentiment several times, and I agree. We don't want the filesystem hint values/mapping to change over time, or it will conflict with data that was written with the previous hints (e.g. if data was written with a "short lifetime" hint suddenly changes to be grouped with a "long lifetime" hint). This is easily avoided with some simple changes now, but harder to fix after this patch has landed. Cheers, Andreas
