From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Date: Tue, 19 Jun 2018 08:42:34 +0200 Subject: [Cluster-devel] [PATCH v9 4/5] gfs2: iomap direct I/O support In-Reply-To: <20180615121922.13237-5-agruenba@redhat.com> References: <20180615121922.13237-1-agruenba@redhat.com> <20180615121922.13237-5-agruenba@redhat.com> Message-ID: <20180619064234.GD24513@lst.de> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit > } else if (flags & IOMAP_WRITE) { > u64 size; > + > + if (flags & IOMAP_DIRECT) > + goto out; > + Maybe add a comment here on why you don't allow block allocations for direct I/O. > + if (flags & IOMAP_DIRECT) { > + ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp); > + release_metapath(&mp); > + if (iomap->type != IOMAP_MAPPED) > + ret = -ENOTBLK; > + } else { > + ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap); > + } A couple too long lines. > } else { > ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp); > release_metapath(&mp); But shouldn't the direct I/O code try to reuse this part anyway? E.g. something like: if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE)) { ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap); } else { ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp); release_metapath(&mp); if ((flags & IOMAP_WRITE) && iomap->type != IOMAP_MAPPED) ret = -ENOTBLK; > + /* fall back to buffered I/O for stuffed files */ > + ret = -ENOTBLK; > + if (gfs2_is_stuffed(ip)) > + goto out; I think we can handle stuffed files in the direct I/O code trivially by copying out the inline data in the iomap. It would be great to just handle this instead of adding fallbacks. > + /* Silently fall back to buffered I/O for stuffed files */ > + if (gfs2_is_stuffed(ip)) > + goto out; Same here. > +static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to) > +{ > + ssize_t ret; > + > + if (iocb->ki_flags & IOCB_DIRECT) { > + ret = gfs2_file_direct_read(iocb, to); > + if (likely(ret != -ENOTBLK)) > + goto out; return ret; > + iocb->ki_flags &= ~IOCB_DIRECT; > + } > + ret = generic_file_read_iter(iocb, to); > +out: > + return ret; return generic_file_read_iter(iocb, to); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from verein.lst.de ([213.95.11.211]:39310 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750917AbeFSGdf (ORCPT ); Tue, 19 Jun 2018 02:33:35 -0400 Date: Tue, 19 Jun 2018 08:42:34 +0200 From: Christoph Hellwig To: Andreas Gruenbacher Cc: cluster-devel@redhat.com, Christoph Hellwig , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v9 4/5] gfs2: iomap direct I/O support Message-ID: <20180619064234.GD24513@lst.de> References: <20180615121922.13237-1-agruenba@redhat.com> <20180615121922.13237-5-agruenba@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180615121922.13237-5-agruenba@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: > } else if (flags & IOMAP_WRITE) { > u64 size; > + > + if (flags & IOMAP_DIRECT) > + goto out; > + Maybe add a comment here on why you don't allow block allocations for direct I/O. > + if (flags & IOMAP_DIRECT) { > + ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp); > + release_metapath(&mp); > + if (iomap->type != IOMAP_MAPPED) > + ret = -ENOTBLK; > + } else { > + ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap); > + } A couple too long lines. > } else { > ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp); > release_metapath(&mp); But shouldn't the direct I/O code try to reuse this part anyway? E.g. something like: if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE)) { ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap); } else { ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp); release_metapath(&mp); if ((flags & IOMAP_WRITE) && iomap->type != IOMAP_MAPPED) ret = -ENOTBLK; > + /* fall back to buffered I/O for stuffed files */ > + ret = -ENOTBLK; > + if (gfs2_is_stuffed(ip)) > + goto out; I think we can handle stuffed files in the direct I/O code trivially by copying out the inline data in the iomap. It would be great to just handle this instead of adding fallbacks. > + /* Silently fall back to buffered I/O for stuffed files */ > + if (gfs2_is_stuffed(ip)) > + goto out; Same here. > +static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to) > +{ > + ssize_t ret; > + > + if (iocb->ki_flags & IOCB_DIRECT) { > + ret = gfs2_file_direct_read(iocb, to); > + if (likely(ret != -ENOTBLK)) > + goto out; return ret; > + iocb->ki_flags &= ~IOCB_DIRECT; > + } > + ret = generic_file_read_iter(iocb, to); > +out: > + return ret; return generic_file_read_iter(iocb, to);