LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Zach Brown <zach.brown@oracle.com>
To: suparna@in.ibm.com
Cc: Jiri Kosina <jikos@jikos.cz>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Ananiev, Leonid I" <leonid.i.ananiev@intel.com>,
	linux-kernel@vger.kernel.org, linux-aio <linux-aio@kvack.org>,
	Chris Mason <chris.mason@oracle.com>,
	Badari Pulavarty <pbadari@us.ibm.com>, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH] aio: fix kernel bug when page is  temporally busy
Date: Fri, 9 Feb 2007 12:02:08 -0500	[thread overview]
Message-ID: <5C55DFF2-6593-41FD-A161-D99331E419E3@oracle.com> (raw)
In-Reply-To: <20070209110555.GA11232@in.ibm.com>


On Feb 9, 2007, at 6:05 AM, Suparna Bhattacharya wrote:

> On Fri, Feb 09, 2007 at 11:40:27AM +0100, Jiri Kosina wrote:
>> On Fri, 9 Feb 2007, Andrew Morton wrote:
>>
>>>> @@ -1204,7 +1204,7 @@ generic_file_aio_read(struct kiocb *iocb,  
>>>> const struct iovec *iov,
>>>>  			do_generic_file_read(filp,ppos,&desc,file_read_actor);
>>>>  			retval += desc.written;
>>>>  			if (desc.error) {
>>>> -				retval = retval ?: desc.error;
>>>> +				retval = desc.error;

I was worried about this too.

> blocking. The high level AIO code (see aio_rw_vect_rety) has the  
> ability
> to handle this.

I had missed this, and yeah, that's some level of comfort.

But I'm not convinced we can guarantee that's safe.  The positive  
return code that aio_rw_vect_retry() sees is telling it that some IO  
has completed and, arguably, that no more IO is in flight.  If we  
return partial progress from generic_file_aio_read() while we have an  
iocb in a wait queue then we are adding yet another invariant.  That  
while an iocb is pending from a previous call down the call chain, we  
can't return a non-aio negative error.  Doing so would cause fs/aio.c  
to complete while there is still an iocb on a wait queue from a  
previous retry attempt.  Right?

I also noticed something just now while poking around these paths to  
see if I could get the start of generic_file_aio_read() to fail when  
it had previously succeeded.  What's to stop another task from racing  
to set O_DIRECT between retries?

That sounds like a pretty hilarious way to get a read retry to fail  
due to buffer misalignment while a previously buffered instance of it  
is still in flight.  Hi-larious.

In thinking about this a discussing it with Chris a bit, I wonder if  
the right fix isn't to refuse changing O_DIRECT via setfl() once any  
IO paths have started on the filp.  Something like:

	filp->frozen_flags |= O_DIRECT

at the start of paths and check it in setfl()?

Are we similarly worried about O_APPEND?

- z

  parent reply	other threads:[~2007-02-09 17:03 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-09  4:29 Ananiev, Leonid I
2007-02-09  4:35 ` Andrew Morton
2007-02-09  5:41   ` Ananiev, Leonid I
2007-02-09  5:52     ` Andrew Morton
2007-02-12 22:52       ` Ananiev, Leonid I
2007-02-12 23:21       ` Ananiev, Leonid I
2007-02-09  7:16     ` Suparna Bhattacharya
2007-02-09  9:52       ` Ananiev, Leonid I
2007-02-09 10:11         ` Jiri Kosina
2007-02-10 18:05         ` Ken Chen
2007-02-10 18:17           ` Ananiev, Leonid I
2007-02-10 18:27           ` Ananiev, Leonid I
2007-02-10 21:57           ` Ananiev, Leonid I
2007-02-15  9:16           ` Ananiev, Leonid I
2007-02-15 18:25             ` Zach Brown
2007-02-15 19:11               ` Ananiev, Leonid I
2007-02-15 19:22                 ` Zach Brown
2007-02-15 21:06                   ` Ananiev, Leonid I
2007-02-15 23:32                   ` Ananiev, Leonid I
2007-02-16  0:01                     ` Zach Brown
2007-02-16 12:18                       ` Ananiev, Leonid I
2007-02-09  9:54 ` Jiri Kosina
2007-02-09 10:14   ` Andrew Morton
2007-02-09 10:40     ` Jiri Kosina
2007-02-09 11:05       ` Suparna Bhattacharya
2007-02-09 11:18         ` Ananiev, Leonid I
2007-02-09 17:02         ` Zach Brown [this message]
2007-02-10 19:36 Ananiev, Leonid I
2007-02-14 17:51 Ananiev, Leonid I
2007-02-15  3:30 ` Andrew Morton
2007-02-15  5:26   ` Ananiev, Leonid I

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5C55DFF2-6593-41FD-A161-D99331E419E3@oracle.com \
    --to=zach.brown@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=chris.mason@oracle.com \
    --cc=jack@suse.cz \
    --cc=jikos@jikos.cz \
    --cc=leonid.i.ananiev@intel.com \
    --cc=linux-aio@kvack.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbadari@us.ibm.com \
    --cc=suparna@in.ibm.com \
    --subject='Re: [PATCH] aio: fix kernel bug when page is  temporally busy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).