Re: Orangefs ABI documentation

[Mike Marshall <hubcap@omnibond.com>]

> If there is (or at least supposed to be) something that prevents completions
> of readdir requests (on unrelated directories, by different processes, etc.)
> out of order, PLEASE SAY SO.  I would really prefer not to have to fight
> the readdir side of that mess; cancels are already bad enough ;-/

Hi Al... your ideas sound good to me, I'll try to get you good
answers on stuff like the above sometime tomorrow...

Thanks!

-Mike

On Wed, Feb 10, 2016 at 7:44 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Feb 09, 2016 at 11:13:28PM +0000, Al Viro wrote:
>> On Tue, Feb 09, 2016 at 10:40:50PM +0000, Al Viro wrote:
>>
>> > And the version in orangefs-2.9.3.tar.gz (your Frankenstein module?) is
>> > vulnerable to the same race.  2.8.1 isn't - it ignores signals on the
>> > cancel, but that means waiting for cancel to be processed (or timed out)
>> > on any interrupted read() before we return to userland.  We can return
>> > to that behaviour, of course, but I suspect that offloading it to something
>> > async (along with freeing the slot used by original operation) would be
>> > better from QoI point of view.
>>
>> That breakage had been introduced between 2.8.5 and 2.8.6 (at some point
>> during the spring of 2012).  AFAICS, all versions starting with 2.8.6 are
>> vulnerable...
>
> Matter of fact, older versions are _also_ broken, but it's much harder
> to trigger; you need the daemon to stall long enough for read() to time
> out, then everything as before.  After 2.8.5 all it takes on the read()
> side is a SIGKILL to read() caller just after the daemon has picked your
> request...
>
> TBH, it's tempting to kill the "wait for cancel to be finished" logics
> completely, mark cancel requests with a flag and stash the slot number
> into them.  Then, if flag is present, let set_op_state_purged() and
> set_op_state_serviced() callers release the slot and drop the request.
> And have wait_for_direct_io() treat the "need to cancel" case as "fire
> and forget" - no waiting for anything, no releasing the slots, just free
> the original op and bugger off.
>
> The set_op_state_purged() part is where it gets delicate - we want to remove
> the sucker from list/hash before dropping it.  AFAICS, it's doable - there's
> nothing in the "release slot and drop the request" that would not be allowed
> under the list/hash spinlocks...
>
> In principle, readdir problem could also be handled in a similar way, but
> there we'd need to have the interrupted readdir itself marked with that
> new flag and left in in-progress hash, so that when the response finally
> arrives it would be found and slot freeing would be handled ;-/  Doable,
> but not fun...
>
> If there is (or at least supposed to be) something that prevents completions
> of readdir requests (on unrelated directories, by different processes, etc.)
> out of order, PLEASE SAY SO.  I would really prefer not to have to fight
> the readdir side of that mess; cancels are already bad enough ;-/