Re: [PATCH 3/6] direct-io: add support for write stream IDs

[Dave Chinner <david@fromorbit.com>]

On Fri, Apr 10, 2015 at 04:50:05PM -0700, Ming Lin wrote:
> On Wed, Mar 25, 2015 at 7:26 AM, Jens Axboe <axboe@kernel.dk> wrote:
> > On 03/24/2015 08:43 PM, Dave Chinner wrote:
> >>
> >> On Tue, Mar 24, 2015 at 09:27:00AM -0600, Jens Axboe wrote:
> >>>
> >>> Get the streamid from the file, if any, and set it on the bio.
> >>>
> >>> Signed-off-by: Jens Axboe <axboe@fb.com>
> >>> ---
> >>>   fs/direct-io.c | 4 ++++
> >>>   1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/fs/direct-io.c b/fs/direct-io.c
> >>> index e181b6b2e297..5d2750346451 100644
> >>> --- a/fs/direct-io.c
> >>> +++ b/fs/direct-io.c
> >>> @@ -77,6 +77,7 @@ struct dio_submit {
> >>>         int reap_counter;               /* rate limit reaping */
> >>>         sector_t final_block_in_request;/* doesn't change */
> >>>         int boundary;                   /* prev block is at a boundary */
> >>> +       int streamid;                   /* Write stream ID */
> >>>         get_block_t *get_block;         /* block mapping function */
> >>>         dio_submit_t *submit_io;        /* IO submition function */
> >>>
> >>> @@ -372,6 +373,8 @@ dio_bio_alloc(struct dio *dio, struct dio_submit
> >>> *sdio,
> >>>
> >>>         sdio->bio = bio;
> >>>         sdio->logical_offset_in_bio = sdio->cur_page_fs_offset;
> >>> +
> >>> +       bio_set_streamid(bio, sdio->streamid);
> >>>   }
> >>>
> >>>   /*
> >>> @@ -1205,6 +1208,7 @@ do_blockdev_direct_IO(int rw, struct kiocb *iocb,
> >>> struct inode *inode,
> >>>         sdio.blkbits = blkbits;
> >>>         sdio.blkfactor = i_blkbits - blkbits;
> >>>         sdio.block_in_file = offset >> blkbits;
> >>> +       sdio.streamid = iocb->ki_filp->f_streamid;
> >>
> >>
> >> If iocb->ki_filp->f_streamid is not set, then it should fall back to
> >> whatever is set on the inode->i_streamid.
> 
> Why should do the fall back?

Because then you have a method of using streams with applications
that aren't aware of streams.

Or perhaps you have a file you know has different access patterns to
the rest of the files in a directory, and you don't want to have to
set the stream on every process that opens and uses that file. e.g.
database writeahead log files (sequential write, never read) vs
database index/table files (random read/write).....

> > Good point, agree. Will make that change.
> 
> That change causes problem for direct IO, for example
> 
> process 1:
> fd = open("/dev/nvme0n1", O_DIRECT...);
> //set stream_id 1
> fadvise(fd, 1, 0, POSIX_FADV_STREAMID);
> pwrite(fd, ....);
> 
> process 2:
> fd = open("/dev/nvme0n1", O_DIRECT...);
> //should be legacy stream_id 0
> pwrite(fd, ....);
> 
> But now process 2 also see stream_id 1, which is wrong.

It's not wrong, your behaviour model is just different You have
defined a process/fd based stream model and not considered
considered that admins and applications might want to use a file
based stream model instead, so applications don't need to even be
aware that write streams are in use...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com