Re: Orangefs ABI documentation

From: Mike Marshall <hubcap@omnibond.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: Orangefs ABI documentation
Date: Sat, 23 Jan 2016 17:36:41 -0500	[thread overview]
Message-ID: <CAOg9mSRwNcXZ2W31Ua-zReYWLS1oQcXL+=5kJuXjwHWUQHQRhg@mail.gmail.com> (raw)
In-Reply-To: <20160123214006.GO17997@ZenIV.linux.org.uk>

AV> IOW, would the following do the right thing?
AV> That would've left us with only one caller of
AV> handle_io_error()...

It works.  With your simplified code all
the needed things still happen: complete and
bufmap_put...

I've never had an error there unless I forgot
to turn on the client-core...

You must be looking for a way to get rid of
another macro <g>...

-Mike

On Sat, Jan 23, 2016 at 4:40 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Jan 23, 2016 at 02:24:51PM -0500, Mike Marshall wrote:
>> OK, I'll get them momentarily...
>>
>> I merged your other patches, and there was a merge
>> conflict I had to work around... you're working from
>> an orangefs tree that lacks one commit I had made
>> last week... my linux-next tree has all your patches
>> through yesterday in it now...
>>
>> I am setting up "the gnarly test" (at home from a VM,
>> though) that should cause a bunch of cancellations,
>> I want to see if I can get
>> wait_for_cancellation_downcall to ever
>> flow past that "if (signal_pending(current)) {"
>> block... if it does, that demonstrate where
>> the comments conflict with the code, right?
>
> Yes...  BTW, speaking of that codepath - how can the second caller of
> handle_io_error() ever get !op_state_serviced(new_op)?  That failure,
> after all, had been in postcopy_buffers(), so the daemon is sitting
> in its write_iter() waiting until we finish copying the data out of
> bufmap; it's too late for sending cancel anyway, is it not?  IOW, would
> the following do the right thing?  That would've left us with only
> one caller of handle_io_error()...
>
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index c585063d..86ba1df 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -245,15 +245,8 @@ populate_shared_memory:
>                                        buffer_index,
>                                        iter,
>                                        new_op->downcall.resp.io.amt_complete);
> -               if (ret < 0) {
> -                       /*
> -                        * put error codes in downcall so that handle_io_error()
> -                        * preserves it properly
> -                        */
> -                       new_op->downcall.status = ret;
> -                       handle_io_error();
> -                       goto out;
> -               }
> +               if (ret < 0)
> +                       goto done_copying;
>         }
>         gossip_debug(GOSSIP_FILE_DEBUG,
>             "%s(%pU): Amount written as returned by the sys-io call:%d\n",
> @@ -263,6 +256,7 @@ populate_shared_memory:
>
>         ret = new_op->downcall.resp.io.amt_complete;
>
> +done_copying:
>         /*
>          * tell the device file owner waiting on I/O that this read has
>          * completed and it can return now.