xfs
[Top] [All Lists]

Re: [PATCH 0/5] splice: locking changes and code refactoring

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 0/5] splice: locking changes and code refactoring
From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Wed, 15 Jan 2014 18:10:27 +0000
Cc: Jens Axboe <axboe@xxxxxxxxx>, Mark Fasheh <mfasheh@xxxxxxxx>, Joel Becker <jlbec@xxxxxxxxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>, Sage Weil <sage@xxxxxxxxxxx>, Steve French <sfrench@xxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140114172033.GU10323@xxxxxxxxxxxxxxxxxx>
References: <20131212181459.994196463@xxxxxxxxxxxxxxxxxxxxxx> <20140113141416.GA30117@xxxxxxxxxxxxx> <20140113235646.GR10323@xxxxxxxxxxxxxxxxxx> <20140114132207.GA25170@xxxxxxxxxxxxx> <20140114172033.GU10323@xxxxxxxxxxxxxxxxxx>
Sender: Al Viro <viro@xxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Jan 14, 2014 at 05:20:33PM +0000, Al Viro wrote:
> And that's less than half of fs/*...  I'm not saying that the current
> situation on the write side is good; hell, just the mess with write/aio_write
> alone is ugly enough - we have
>       * a bunch of file_operations without ->aio_write(); simple enough.
>       * a bunch with ->write == do_sync_write.  Also simple.
>       * several with NULL ->write and non-NULL ->aio_write(); same as
> do_sync_write() for ->write (socket, android/logger, kmsg, macvtap)
>       * several with ->aio_write being an optimized equivalent of
> do_sync_write() (blackhole for /dev/null and /dev/zero, error for bad_inode)
>       * 9p cached with its "oh, but if we have O_DIRECT we want ->write()
> to be different" (why not use a separate file_operations, then?  It's not
> as if ->open() couldn't switch to it if it sees O_DIRECT...)
>       * two infinibad things (ipath and qib), with completely unrelated
> semantics on write(2) and writev(2) (the latter shared with aio).  As in
> "writev() of a single-element iovec array does things that do not even
> resemble what write() of the same data would've done".  Yes, really - check
> for yourself.
>       * snd_pcm - hell knows; it might be that it tries to collect the
> data from iovec and push it in one go, as if it was a single write, but
> then it might be something as bogus as what ipath is doing...
>       * gadgetfs - hell knows; ep_write() seems to be doing something
> beyond what ep_aio_write() does, but I haven't traced them down the call
> chain...  That one, BTW, won't be fun for splice - looks like it cares
> about datagram boundaries a lot, so it's not obvious what the semantics
> should be.
>       * lustre.  I _think_ do_sync_write() would work there, but I'm might
> be easily missing something in all those layers of obfusca^Wgood software
> development practices.

BTW, ->read/->aio_read situation is only slighlty better - of file_operations
instances that have ->aio_read, most have do_sync_read() for ->read() (as
they ought to).  Exceptions:
        * 9p O_DIRECT (again)
        * NULL ->read where do_sync_read ought to be (socket, macvtap)
        * optimized ->read (/dev/zero, /dev/null, bad_inode)
        * snd_pcm - magic.  It (and its ->aio_write counterpart) wants exactly
one iovec per channel.  IOW, it's not a general-purpose ->aio_{read,write}
at all - it's a magic API shoehorned into readv(2)/writev(2) (and aio
IOCB_CMD_P{READ,WRITE}V as well).
        * lustre - probably could live with do_sync_read(), but there might
be stack footprint considerations or some really weird magic going on
(the difference is that instead of iocb on stack they appear to be using
per-thread one allocated on heap and hashed by pid, of all things).
It's really weird - they end up doing repeated hash lookups for that
per-thread wastebasket of a structure on different levels of call chain.
Looks like they have swept a lot of local variables of a lot of functions
into that thing; worse, it appears to be one of several dynamically allocated
bits of that thing, hidden behind a bunch of wrappers...  Overall feel is
Lovecraftian, complete with lurking horrors of the deep...  BTW, its ->aio_read
would better never return -EIOCBQUEUED - its ->read does *not* wait for
completion of iocb it has submitted.
        * gadgetfs - it appears to be seriously datagram-oriented; basically,
they want to reduce readv/writev to read/write, not the other way round.

> BTW, speaking of ->aio_write() - why the devil do we pass the pos
> argument (long long, at that) separately, when all call sites provably
> have it equal to iocb->ki_pos?  If nothing else, on a bunch of architectures
> it makes the difference between passing arguments in registers and spilling
> them on stack; moreover, if we do something and only then call
> generic_file_aio_write(), we get to propagate it all way down.  And
> generic_file_aio_write() has had explicit BUG_ON(iocb->ki_pos != pos)
> since 2.5.55, for crying out loud...

The same goes for ->aio_read() (except for s/2.5.55/2.5.39/)...

<Prev in Thread] Current Thread [Next in Thread>