From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755329AbaIPToQ (ORCPT ); Tue, 16 Sep 2014 15:44:16 -0400 Received: from mail-la0-f53.google.com ([209.85.215.53]:56939 "EHLO mail-la0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755050AbaIPToP (ORCPT ); Tue, 16 Sep 2014 15:44:15 -0400 MIME-Version: 1.0 In-Reply-To: References: Date: Tue, 16 Sep 2014 15:44:13 -0400 Message-ID: Subject: Re: [PATCH 4/7] O_NONBLOCK flag for readv2/preadv2 From: Milosz Tanski To: Jeff Moyer Cc: LKML , Christoph Hellwig , "linux-fsdevel@vger.kernel.org" , linux-aio@kvack.org, Mel Gorman , Volker Lendecke , Tejun Heo Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 16, 2014 at 3:19 PM, Jeff Moyer wrote: > Milosz Tanski writes: > >> Filesystems that generic_file_read_iter will not be allowed to perform >> non-blocking reads. This only will read data if it's in the page cache and if >> there is no page error (causing a re-read). >> >> Signed-off-by: Milosz Tanski > >> @@ -1662,6 +1676,10 @@ no_cached_page: >> goto out; >> } >> goto readpage; >> + >> +would_block: >> + error = -EAGAIN; >> + goto out; >> } > > Why did you put the wouldblock label inside the loop? That should be > pushed down to just above out, and then you can get rid of the goto. When I put the code outside the loop it actually looked worse (imo): } goto out; would_block: error = -EAGAIN; out: ... > > Other than that, it looks like you put the check in all the right places > in that function. > >> out: >> @@ -1697,6 +1715,9 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter, int flags) >> size_t count = iov_iter_count(iter); >> loff_t size; >> >> + if (flags & O_NONBLOCK) >> + return -EAGAIN; >> + > > If a program is attempting non-blocking reads on a file opened with > O_DIRECT, I think returning -EAGAIN is very misleading. Better to > return -EINVAL in this case, and maybe check that earlier in the stack? Point taken and I can fix this for the next version further up the stack. A longer term question is how the flags the file is open with interact with the read/write flags ... since I imagine folks will want to add other flags (like force a SYNC write). > > Cheers, > Jeff -- Milosz Tanski CTO 16 East 34th Street, 15th floor New York, NY 10016 p: 646-253-9055 e: milosz@adfin.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Milosz Tanski Subject: Re: [PATCH 4/7] O_NONBLOCK flag for readv2/preadv2 Date: Tue, 16 Sep 2014 15:44:13 -0400 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: LKML , Christoph Hellwig , "linux-fsdevel@vger.kernel.org" , linux-aio@kvack.org, Mel Gorman , Volker Lendecke , Tejun Heo To: Jeff Moyer Return-path: In-Reply-To: Sender: owner-linux-aio@kvack.org List-Id: linux-fsdevel.vger.kernel.org On Tue, Sep 16, 2014 at 3:19 PM, Jeff Moyer wrote: > Milosz Tanski writes: > >> Filesystems that generic_file_read_iter will not be allowed to perform >> non-blocking reads. This only will read data if it's in the page cache and if >> there is no page error (causing a re-read). >> >> Signed-off-by: Milosz Tanski > >> @@ -1662,6 +1676,10 @@ no_cached_page: >> goto out; >> } >> goto readpage; >> + >> +would_block: >> + error = -EAGAIN; >> + goto out; >> } > > Why did you put the wouldblock label inside the loop? That should be > pushed down to just above out, and then you can get rid of the goto. When I put the code outside the loop it actually looked worse (imo): } goto out; would_block: error = -EAGAIN; out: ... > > Other than that, it looks like you put the check in all the right places > in that function. > >> out: >> @@ -1697,6 +1715,9 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter, int flags) >> size_t count = iov_iter_count(iter); >> loff_t size; >> >> + if (flags & O_NONBLOCK) >> + return -EAGAIN; >> + > > If a program is attempting non-blocking reads on a file opened with > O_DIRECT, I think returning -EAGAIN is very misleading. Better to > return -EINVAL in this case, and maybe check that earlier in the stack? Point taken and I can fix this for the next version further up the stack. A longer term question is how the flags the file is open with interact with the read/write flags ... since I imagine folks will want to add other flags (like force a SYNC write). > > Cheers, > Jeff -- Milosz Tanski CTO 16 East 34th Street, 15th floor New York, NY 10016 p: 646-253-9055 e: milosz@adfin.com -- To unsubscribe, send a message with 'unsubscribe linux-aio' in the body to majordomo@kvack.org. For more info on Linux AIO, see: http://www.kvack.org/aio/ Don't email: aart@kvack.org