Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361192 > unrolled thread

Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-20 02:30 +0100
    Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async  fsync Al Viro <viro@ZenIV.linux.org.uk> - 2016-03-20 02:30 +0100
      Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-20 02:50 +0100
        Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async  fsync Al Viro <viro@ZenIV.linux.org.uk> - 2016-03-20 03:00 +0100
          Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-20 03:10 +0100

#1361192 — Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Mar 14, 2016 at 10:17 AM, Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> I had some time last week to make an aio openat do what it can in
> submit context.  The results are an improvement: when openat is handled
> in submit context it completes in about half the time it takes compared
> to the round trip via the work queue, and it's not terribly much code
> either.

This looks good to me, and I do suspect that any of these aio paths
should strive to have a synchronous vs threaded model. I think that
makes the whole thing much more interesting from a performance
standpoint.

I still think the aio interface is really nasty,  but this together
with the table-based approach you posted earlier does make me a _lot_
happier about the implementation.It just looks way less hacky, and now
it ends up exposing a rather more clever implementation too.

             Linus

[toc] | [next] | [standalone]

#1361193 — Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync

On Sat, Mar 19, 2016 at 06:20:24PM -0700, Linus Torvalds wrote:
> On Mon, Mar 14, 2016 at 10:17 AM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> >
> > I had some time last week to make an aio openat do what it can in
> > submit context.  The results are an improvement: when openat is handled
> > in submit context it completes in about half the time it takes compared
> > to the round trip via the work queue, and it's not terribly much code
> > either.
> 
> This looks good to me, and I do suspect that any of these aio paths
> should strive to have a synchronous vs threaded model. I think that
> makes the whole thing much more interesting from a performance
> standpoint.

Umm...  You do realize that LOOKUP_RCU in flags does *NOT* guarantee that
it won't block, right?  At the very least one would need to refuse to
fall back on non-RCU mode without a full restart.  Furthermore, vfs_open()
itself can easily block.

So this new LOOKUP flag makes no sense, and it's in the just about _the_
worst place possible for adding special cases with ill-defined semantics -
do_last() is already far too convoluted and needs untangling, not adding
half-assed kludges.

[toc] | [prev] | [next] | [standalone]

#1361197

On Sat, Mar 19, 2016 at 6:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Umm...  You do realize that LOOKUP_RCU in flags does *NOT* guarantee that
> it won't block, right?  At the very least one would need to refuse to
> fall back on non-RCU mode without a full restart.

It actually does seem to do that, although in an admittedly rather
questionable way.

I think it should use path_openat() rather than do_filp_open(), but
passing in LOOKUP_RCU to do_filp_open() actually does work: it just
means that the retry after ECHILD/ESTALE will just do it *again* with
LOOKUP_RCU. It won't fall back to non-rcu mode, it just won't or in
the LOOKUP_RCU flag that is already set.

So I agree that it should be cleaned up, but the basic model seems
fine. I'm sure you're right about do_last() not necessarily being the
best place either. But that doesn't really change that the approach
seems *much* better than the old unconditional "do in a work queue".

Also, the whole "no guarantees of never blocking" is a specious argument.

Just copying the iocb from user space can block. Copying the pathname
likewise (or copying the iovec in the case of reads and writes). So
the aio interface at no point is "guaranteed to never block". Blocking
will happen. You can block on allocating the "struct file", or on
extending the filp table.

In the end it's about _performance_, and if the performance is better
with very unlikely blocking synchronous calls, then that's the right
thing to do.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1361200 — Re: aio openat Re: [PATCH 07/13] aio: enabled thread based async fsync

On Sat, Mar 19, 2016 at 06:45:19PM -0700, Linus Torvalds wrote:

> It actually does seem to do that, although in an admittedly rather
> questionable way.
> 
> I think it should use path_openat() rather than do_filp_open(), but
> passing in LOOKUP_RCU to do_filp_open() actually does work: it just
> means that the retry after ECHILD/ESTALE will just do it *again* with
> LOOKUP_RCU. It won't fall back to non-rcu mode, it just won't or in
> the LOOKUP_RCU flag that is already set.

What would make unlazy_walk() fail?  And if it succeeds, you are not
in RCU mode anymore *without* restarting from scratch...

[toc] | [prev] | [next] | [standalone]

#1361207

On Sat, Mar 19, 2016 at 6:55 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> What would make unlazy_walk() fail?  And if it succeeds, you are not
> in RCU mode anymore *without* restarting from scratch...

I don't see your point.

You don't want to be in RCU mode any more. You want to either succeed
or fail with ECHILD/ESTALE. Then, in the failure case, you go to the
thread.

What I meant by restarting was the restart that do_filp_open() does,
and there it just restarts with "op->lookup_flags", which has
RCU_LOOKUP still set, so it would just try to do the RCU lookup again.

But I actually notice now that Ben actually disabled that restart if
LOOKUP_RCU was set, so that ends up not even happening.

Anyway, I'm not saying it's polished and pretty. I think the changes
to do_filp_open() are a bit silly, and the code should just use
path_openat() directly. Possibly using a new helper (ie perhaps just
introduce a "rcu_filp_openat()" thing). But from a design perspective,
I think this all looks fine.

              Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web