On 12/14/2012 07:40 AM, Maxim V. Patlasov wrote:
> Hi Brian,
>
> 12/13/2012 11:51 PM, Brian Foster пишет:
>> [cut]
>> Before getting into that, I made some adjustments to the original patch
>> to use your new fuse_io_priv structure in place of an iocb, primarily to
>> decouple the request submission mechanism from the type of the user
>> request. The highlighted differences are:
>>
>> - Updated fuse_io_priv with an async field and file pointer to preserve
>> the current style of interface (i.e., use this instead of iocb).
>> - Trigger the type of request submission based on the async field.
>> - Reintroduce the wait in fuse_direct_IO(), but I also pulled up the
>> fuse_write_update_size() call out of __fuse_direct_write() to make the
>> separate paths more consistent.
>
> Sounds reasonable.
>
>> This could probably use more cleanups (i.e., it morphs your fuse_io_priv
>> into more of a generic I/O descriptor), but illustrates the idea.
>
> The patch you provided looks fine. Do you have any particular (further)
> cleanups in mind? I'm going to resend initial patch-set with your last
> patch applied. If you provide more ideas about improving it, I'll be
> glad to consider and integrate them as well.
>
Great! I was considering some minor things such as renaming the
structure, turning the async field into a generic flags field with an
async bit, perhaps an initialization helper for the io_priv would be
useful, etc.
I'm not totally sold on any of that and I still need to take a more
detailed look at the overall set as well as repeat some of your perf.
tests on gluster and real hardware, so perhaps it is best to see v2
shake out first and I'll follow up from there...
>> [cut]
>>> The problem is that FUSE tends to be the only user of this 'feature'.
>>> I'm considering one-line change of aio_complete():
>>>
>>> - if (is_sync_kiocb(iocb)) {
>>> + if (!iocb->ki_ctx) {
>>>
>>> Though, not sure whether Miklos, Al Viro, Benjamin and others will
>>> accept it... Let's try if you're OK about this approach. At least this
>>> will cover all fuse dio use-cases (including libaio extending file) and
>>> makes fuse code significantly cleaner.
>>>
>> Interesting idea. It seems like it could work to me but I'm not very
>> familiar with that code and it might not be worth cluttering that
>> interface for this particular case.
>
> Yes, exactly. Moreover, after looking at aio.h closer:
>
>> struct kioctx *ki_ctx; /* may be NULL for sync ops */
>
> I realized that that would be rather hack: sync ops doesn't use ki_ctx
> for now, but this can change in the future.
>
Right, it seems like that wouldn't break !sync aio, but could set a
landmine for future sync aio uses (not to mention confusing the
wait_on_sync_kiocb() semantics).
>> I brought this up briefly with Jeff
>> Moyer and he pointed me at this set (I just saw zab's mail pointing this
>> out as well :):
>>
>> https://lkml.org/lkml/2012/10/22/321
>>
>> ... which looks like it could help. Perhaps we could use that new type
>> of kiocb with a custom callback to wake a blocking thread. That said, I
>> wonder if we could do that anyways; i.e., define our own
>> wait_on_fuse_async() type function that that waits on the number of
>> in-flight requests in the fuse_priv_io. I guess we'd have to complicate
>> the refcounting with that type of approach, though.
>
> I've considered using that new API (kernel aio) but refrained from it
> deliberately. The reasons were similar to ones you listed above: why
> should we invent another synchronization mechanism if
> wait_on_sync_kiocb() does exactly what we need? Now, after more
> thinking, I tend to think that using that API is doable, but the way how
> we could do it is too clumsy. Please let me know if you need more details.
>
That sounds reasonable to me. I was just trying to consider alternate
ideas. It does seem unnecessary to jump through hoops when we can
accomplish the same behavior with a cleaner interface. Thanks!
Brian
> Thanks,
> Maxim
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
