Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752766AbcDORhI (ORCPT ); Fri, 15 Apr 2016 13:37:08 -0400 Received: from mga04.intel.com ([192.55.52.120]:45104 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751916AbcDORhF (ORCPT ); Fri, 15 Apr 2016 13:37:05 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,488,1455004800"; d="scan'208";a="85932641" From: "Verma, Vishal L" To: "jmoyer@redhat.com" CC: "linux-kernel@vger.kernel.org" , "linux-block@vger.kernel.org" , "hch@infradead.org" , "xfs@oss.sgi.com" , "linux-nvdimm@ml01.01.org" , "linux-mm@kvack.org" , "viro@zeniv.linux.org.uk" , "akpm@linux-foundation.org" , "axboe@fb.com" , "linux-fsdevel@vger.kernel.org" , "linux-ext4@vger.kernel.org" , "Wilcox, Matthew R" , "david@fromorbit.com" , "jack@suse.cz" Subject: Re: [PATCH v2 5/5] dax: handle media errors in dax_do_io Thread-Topic: [PATCH v2 5/5] dax: handle media errors in dax_do_io Thread-Index: AQHRlznSmdVyB28MyEyyQ/rUxU8Y+Z+LwbqA Date: Fri, 15 Apr 2016 17:37:02 +0000 Message-ID: <1460741821.3012.11.camel@intel.com> References: <1459303190-20072-1-git-send-email-vishal.l.verma@intel.com> <1459303190-20072-6-git-send-email-vishal.l.verma@intel.com> <1460739288.3012.3.camel@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.176.69] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u3FHbD9k004122 Content-Length: 2902 Lines: 81 On Fri, 2016-04-15 at 13:11 -0400, Jeff Moyer wrote: > "Verma, Vishal L" writes: > > > > > On Fri, 2016-04-15 at 12:11 -0400, Jeff Moyer wrote: > > > > > > Vishal Verma writes: > > > > > > > > + if (IS_DAX(inode)) { > > > > + ret = dax_do_io(iocb, inode, iter, offset, > > > > blkdev_get_block, > > > >   NULL, DIO_SKIP_DIO_COUNT); > > > > - return __blockdev_direct_IO(iocb, inode, > > > > I_BDEV(inode), > > > > iter, offset, > > > > + if (ret == -EIO && (iov_iter_rw(iter) == > > > > WRITE)) > > > > + ret_saved = ret; > > > > + else > > > > + return ret; > > > > + } > > > > + > > > > + ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), > > > > iter, offset, > > > >       blkdev_get_block, NULL, > > > > NULL, > > > >       DIO_SKIP_DIO_COUNT); > > > > + if (ret < 0 && ret_saved) > > > > + return ret_saved; > > > > + > > > Hmm, did you just break async DIO?  I think you did!  :) > > > __blockdev_direct_IO can return -EIOCBQUEUED, and you've now > > > turned > > > that > > > into -EIO.  Really, I don't see a reason to save that first > > > -EIO.  The > > > same applies to all instances in this patch. > > The reason I saved it was if __blockdev_direct_IO fails for some > > reason, we should return the original cause o the error, which was > > an > > EIO.. i.e. we shouldn't be hiding the EIO if the direct_IO fails > > with > > something else.. > OK. > > > > > But, how does _EIOCBQUEUED work? Maybe we need an exception for it? > For async direct I/O, only the setup phase of the I/O is performed > and > then we return to the caller.  -EIOCBQUEUED signifies this. > > You're heading towards code that looks like this: > >         if (IS_DAX(inode)) { >                 ret = dax_do_io(iocb, inode, iter, offset, > blkdev_get_block, >                                 NULL, DIO_SKIP_DIO_COUNT); >                 if (ret == -EIO && (iov_iter_rw(iter) == WRITE)) >                         ret_saved = ret; >                 else >                         return ret; >         } > >         ret = __blockdev_direct_IO(iocb, inode, I_BDEV(inode), iter, > offset, >                                     blkdev_get_block, NULL, NULL, >                                     DIO_SKIP_DIO_COUNT); >         if (ret < 0 && ret != -EIOCBQUEUED && ret_saved) >                 return ret_saved; > > There's a lot of special casing here, so you might consider adding > comments. Correct - maybe we should reconsider wrapper-izing this? :) Thanks for the explanation and for catching this. I'll fix it for the next revision. > > Cheers, > Jeff