From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-lb0-f175.google.com ([209.85.217.175]:36691 "EHLO
	mail-lb0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754760AbcAWWgm (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Sat, 23 Jan 2016 17:36:42 -0500
Received: by mail-lb0-f175.google.com with SMTP id oh2so57630650lbb.3
        for <linux-fsdevel@vger.kernel.org>; Sat, 23 Jan 2016 14:36:42 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <20160123214006.GO17997@ZenIV.linux.org.uk>
References: <20160122183720.GB17997@ZenIV.linux.org.uk>
	<CAOg9mSTCkr5_or_VSOHnNSsVqd5S=ENuGYBsJkgitqgworN9Eg@mail.gmail.com>
	<CAOg9mSRoEFvFkdJmEmFOO_ErT7X3Q51=X=HP5Li6g-dVFANOOg@mail.gmail.com>
	<20160122200442.GF17997@ZenIV.linux.org.uk>
	<CAOg9mSTPbx0231RoERbbdeyR+nDCD4wGHHFKAzxmowai8GtNgg@mail.gmail.com>
	<20160123001202.GJ17997@ZenIV.linux.org.uk>
	<20160123012808.GK17997@ZenIV.linux.org.uk>
	<CAOg9mST8rpzSs4FGB=7yiGJ2PZBqw2s=H1ozQBiWjDF232ZnhQ@mail.gmail.com>
	<20160123191055.GN17997@ZenIV.linux.org.uk>
	<CAOg9mSR667SdVvR323MOaqEs0DVGKcxr8e0tyx=Bu_=ENops0g@mail.gmail.com>
	<20160123214006.GO17997@ZenIV.linux.org.uk>
Date: Sat, 23 Jan 2016 17:36:41 -0500
Message-ID: <CAOg9mSRwNcXZ2W31Ua-zReYWLS1oQcXL+=5kJuXjwHWUQHQRhg@mail.gmail.com>
Subject: 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>
Content-Type: text/plain; charset=UTF-8
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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.