On Thu, May 10, 2007 at 04:11:01PM +1000, Timothy Shimmin wrote:
> Hi Dave,
>
> --On 8 May 2007 4:51:26 PM +1000 David Chinner <dgc@xxxxxxx> wrote:
>
> >
> >Back in 2.6.13, unwritten extent conversion was changed to be done
> >via a workqueue because we can't do conversion in interrupt context
> >(AIO issue). The problem was that the changes extent conversion to
> >run asynchronously w.r.t I/o completion.
>
> Oh ok, and at the same time they used the workqueue also (apart
> from AIO) for synchronous direct writes even though they didn't have to.
> i.e the existing comment:
> * This is not necessary for synchronous direct I/O, but we do
> * it anyway to keep the code uniform and simpler.
Yes, exactly.
> So you were tossing up whether to flush the queue as in the patch given
> or to effectively call the code of xfs_end_bio_unwritten to
> do the unwritten extent conversion straight away.
> Hmmm....I dunno :)
> Does it matter? What are the pros and cons? :)
I think with async buffered writes we are doing I/O completion in
IRQ context as well so it seems to me that we have to push the
unwritten extent conversion off to a workqueue in that case.
I don't think there's any great overhead from flushing only when
we are doing sync dio writes - all that calling
xfs_end_bio_unwritten() directly saves us is a couple of context
switches. However, that could promote I/o completion ahead of
other I/Os waiting in the workqueue....
I think I'm convincing myself that the workqueue flush is the
correct thing to do here ;)
> Does it matter if we flush the whole queue now or later?
We have to wait for it to complete, and that's what the flush does;
it waits for the queued work up to the flush entrance sequence
to complete. It's really the only way we can wait for a specific
item in a workqueue to be run. So yes, it needs to be run now,
not later.
> Is it nicer/simpler for this to always happen in the queue?
I think so.
> Is it a bit silly to queue and immediately flush?
I think that's the way you're supposed to do things ;)
> * Possible typo in comment:
> s/passed to use to determine/passed to us to determine/
>
> * Don't really need the "? 1 : 0"
> is_sync_kiocb(iocb) ? 1 : 0
> =>
> is_sync_kiocb(iocb)
Right - I'll fix that.
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
|