Re: [PATCH 0/4] Page based O_DIRECT v2

[Jens Axboe <jens.axboe@oracle.com>]

On Wed, Aug 19 2009, Boaz Harrosh wrote:
> On 08/18/2009 11:34 AM, Jens Axboe wrote:
> > Hi,
> > 
> > Updated patchset for page based O_DIRECT. I didn't include the
> > loop bits this time, lets focus on getting these core bits into
> > shape and then loop is easily patchable on top of this.
> > 
> > Changes since last post:
> > 
> > - Changed do_dio() to generic_file_direct_IO() as per Christophs
> >   suggestion.
> > - Split the first patch into two parts. One simply adds dio_args
> >   and maintains the current code, the next has the functional change
> >   but without changing file systems (except NFS).
> > - Add ->rw to dio_args (Christoph).
> > - A locking fixup. Not really related, but should be fixed up anyways.
> > 
> > There are at least two pending things to work on:
> > 
> > 1) NFS is still broken, I get a crash in freeing some data that
> >    is not related to the pages. Will debug this.
> > 2) As Christoph suggested, we need some way to wait for a dio
> >    when all segments are submitted. Currently it waits for each
> >    segment. Not sure how best to solve this issue, will think a
> >    bit more about this. Basically we need to pass down the wait
> >    list to the generic_file_direct_IO() and have that do the
> >    queue kick and wait.
> > 
> 
> Jens hi.
> 
> I please have some basic question on the subject?
> 
> [1]
> So before, the complete iovec from user mode could potentially be
> submitted in a single request, depending on the implementor.
> With new code, each iovec entry is broken to it's few pages and
> is submitted as a separate request. This might not be bad for
> block based devices that could see these segments merged back by the
> IO elevator. But what about the other implementers that see a
> grate performance boost in the current scatter-gather nature of the
> iovec API. It's almost as if the application was calling the kernel
> for each segment separately.
> 
> I wish you would use a more generic page carrier then page-* array.
> and submit the complete iovec at once.
> 
> We used to use scatter-lists but these are best only used inside DMA
> engines and Drivers as they are more then 2 times too big. The ideal for
> me is the bio_vec array as used inside a bio. scatter-list has all these
> helpers, iterators, and wrappers, which bio_vec do not, so I don't know
> what the best choice is.
> 
> But your current solution, (from inspection only I have not tested any of
> this), might mean a grate performance degradation for some work scenarios.
> For example a user-mode app the gathers lots of small memory sources and
> hopes to write it as a single very large on-the-wire-NFS-write , might find
> itself writing lots of small on-the-wire-NFS-writes.

I fully agree, see also the discussion with Christoph. One way would
indeed be to pass in an array of page map + offset, another would be to
pass something back to enable kicking + waiting on the IO. Haven't
looked in either direction yet, but I hope to do so Very Soon.

> [2]
> Please address linux-fsdevel on these patches. lkml is so crowded and after
> all these files do sit in fs/

Sure, will CC linux-fsdevel next time too.

-- 
Jens Axboe