| Message ID | 2cd1301404ec7cf1e39c8f11a01a4302f1460ad6.1539195310.git.ernesto.mnd.fernandez@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] hfsplus: fix return value of hfsplus_get_block() | expand |
On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > Direct writes to empty inodes fail with EIO. The generic direct-io code > is in part to blame (a patch has been submitted as "direct-io: allow > direct writes to empty inodes"), but hfsplus is worse affected than the > other filesystems because the fallback to buffered I/O doesn't happen. > Could you please share more detailed explanation of the patch that affects the HFS+ behavior? It's hard to follow what patch you mean. Thanks, Vyacheslav Dubeyko. > The problem is the return value of hfsplus_get_block() when called with > !create. Change it to be more consistent with the other modules. > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > --- > fs/hfsplus/extents.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c > index 8a8893d522ef..a930ddd15681 100644 > --- a/fs/hfsplus/extents.c > +++ b/fs/hfsplus/extents.c > @@ -237,7 +237,9 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock, > ablock = iblock >> sbi->fs_shift; > > if (iblock >= hip->fs_blocks) { > - if (iblock > hip->fs_blocks || !create) > + if (!create) > + return 0; > + if (iblock > hip->fs_blocks) > return -EIO; > if (ablock >= hip->alloc_blocks) { > res = hfsplus_file_extend(inode, false);
On Thu, Oct 11, 2018 at 11:05:36AM -0700, Viacheslav Dubeyko wrote: > On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > > Direct writes to empty inodes fail with EIO. The generic direct-io code > > is in part to blame (a patch has been submitted as "direct-io: allow > > direct writes to empty inodes"), but hfsplus is worse affected than the > > other filesystems because the fallback to buffered I/O doesn't happen. > > > > Could you please share more detailed explanation of the patch that > affects the HFS+ behavior? It's hard to follow what patch you mean. It was sent to linux-fsdevel: "direct-io: allow direct writes to empty inodes." > > Thanks, > Vyacheslav Dubeyko. > > > The problem is the return value of hfsplus_get_block() when called with > > !create. Change it to be more consistent with the other modules. > > > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > --- > > fs/hfsplus/extents.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c > > index 8a8893d522ef..a930ddd15681 100644 > > --- a/fs/hfsplus/extents.c > > +++ b/fs/hfsplus/extents.c > > @@ -237,7 +237,9 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock, > > ablock = iblock >> sbi->fs_shift; > > > > if (iblock >= hip->fs_blocks) { > > - if (iblock > hip->fs_blocks || !create) > > + if (!create) > > + return 0; > > + if (iblock > hip->fs_blocks) > > return -EIO; > > if (ablock >= hip->alloc_blocks) { > > res = hfsplus_file_extend(inode, false); > >
On Thu, 2018-10-11 at 16:40 -0300, Ernesto A. Fernández wrote: > On Thu, Oct 11, 2018 at 11:05:36AM -0700, Viacheslav Dubeyko wrote: > > On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > > > Direct writes to empty inodes fail with EIO. The generic direct-io code > > > is in part to blame (a patch has been submitted as "direct-io: allow > > > direct writes to empty inodes"), but hfsplus is worse affected than the > > > other filesystems because the fallback to buffered I/O doesn't happen. > > > > > > > Could you please share more detailed explanation of the patch that > > affects the HFS+ behavior? It's hard to follow what patch you mean. > > It was sent to linux-fsdevel: "direct-io: allow direct writes to empty > inodes." > The git provides the commit ID and the annotation for every commit. It will be great to see these details. Could you please share this information? Thanks, Vyacheslav Dubeyko.
On Thu, Oct 11, 2018 at 05:38:35PM -0700, Viacheslav Dubeyko wrote: > On Thu, 2018-10-11 at 16:40 -0300, Ernesto A. Fernández wrote: > > On Thu, Oct 11, 2018 at 11:05:36AM -0700, Viacheslav Dubeyko wrote: > > > On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > > > > Direct writes to empty inodes fail with EIO. The generic direct-io code > > > > is in part to blame (a patch has been submitted as "direct-io: allow > > > > direct writes to empty inodes"), but hfsplus is worse affected than the > > > > other filesystems because the fallback to buffered I/O doesn't happen. > > > > > > > > > > Could you please share more detailed explanation of the patch that > > > affects the HFS+ behavior? It's hard to follow what patch you mean. > > > > It was sent to linux-fsdevel: "direct-io: allow direct writes to empty > > inodes." > > > > The git provides the commit ID and the annotation for every commit. It > will be great to see these details. Could you please share this > information? The patch hasn't been merged yet. > > Thanks, > Vyacheslav Dubeyko. > >
On Thu, 2018-10-11 at 21:48 -0300, Ernesto A. Fernández wrote: > On Thu, Oct 11, 2018 at 05:38:35PM -0700, Viacheslav Dubeyko wrote: > > On Thu, 2018-10-11 at 16:40 -0300, Ernesto A. Fernández wrote: > > > On Thu, Oct 11, 2018 at 11:05:36AM -0700, Viacheslav Dubeyko wrote: > > > > On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > > > > > Direct writes to empty inodes fail with EIO. The generic direct-io code > > > > > is in part to blame (a patch has been submitted as "direct-io: allow > > > > > direct writes to empty inodes"), but hfsplus is worse affected than the > > > > > other filesystems because the fallback to buffered I/O doesn't happen. > > > > > > > > > > > > > Could you please share more detailed explanation of the patch that > > > > affects the HFS+ behavior? It's hard to follow what patch you mean. > > > > > > It was sent to linux-fsdevel: "direct-io: allow direct writes to empty > > > inodes." > > > > > > > The git provides the commit ID and the annotation for every commit. It > > will be great to see these details. Could you please share this > > information? > > The patch hasn't been merged yet. > I am not completely sure how to review the patch in such case. I believe that the best strategy is to wait till the mentioned patch will be merged and to resend this patch again. Thanks, Vyacheslav Dubeyko.
On Fri, Oct 12, 2018 at 11:43:06AM -0700, Viacheslav Dubeyko wrote: > On Thu, 2018-10-11 at 21:48 -0300, Ernesto A. Fernández wrote: > > On Thu, Oct 11, 2018 at 05:38:35PM -0700, Viacheslav Dubeyko wrote: > > > On Thu, 2018-10-11 at 16:40 -0300, Ernesto A. Fernández wrote: > > > > On Thu, Oct 11, 2018 at 11:05:36AM -0700, Viacheslav Dubeyko wrote: > > > > > On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > > > > > > Direct writes to empty inodes fail with EIO. The generic direct-io code > > > > > > is in part to blame (a patch has been submitted as "direct-io: allow > > > > > > direct writes to empty inodes"), but hfsplus is worse affected than the > > > > > > other filesystems because the fallback to buffered I/O doesn't happen. > > > > > > > > > > > > > > > > Could you please share more detailed explanation of the patch that > > > > > affects the HFS+ behavior? It's hard to follow what patch you mean. > > > > > > > > It was sent to linux-fsdevel: "direct-io: allow direct writes to empty > > > > inodes." > > > > > > > > > > The git provides the commit ID and the annotation for every commit. It > > > will be great to see these details. Could you please share this > > > information? > > > > The patch hasn't been merged yet. > > > > I am not completely sure how to review the patch in such case. I believe > that the best strategy is to wait till the mentioned patch will be > merged and to resend this patch again. You can just ignore the direct-io patch if you find it confusing. It's a separate issue, and the hfs/hfsplus patches are actually easier to test by themselves. > Thanks, > Vyacheslav Dubeyko. > > >
On Wed, 2018-10-10 at 15:23 -0300, Ernesto A. Fernández wrote: > Direct writes to empty inodes fail with EIO. The generic direct-io code > is in part to blame (a patch has been submitted as "direct-io: allow > direct writes to empty inodes"), but hfsplus is worse affected than the > other filesystems because the fallback to buffered I/O doesn't happen. > > The problem is the return value of hfsplus_get_block() when called with > !create. Change it to be more consistent with the other modules. > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > --- > fs/hfsplus/extents.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c > index 8a8893d522ef..a930ddd15681 100644 > --- a/fs/hfsplus/extents.c > +++ b/fs/hfsplus/extents.c > @@ -237,7 +237,9 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock, > ablock = iblock >> sbi->fs_shift; > > if (iblock >= hip->fs_blocks) { > - if (iblock > hip->fs_blocks || !create) > + if (!create) > + return 0; > + if (iblock > hip->fs_blocks) > return -EIO; > if (ablock >= hip->alloc_blocks) { > res = hfsplus_file_extend(inode, false); Looks good. Finally, I think it should work properly. Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com> Thanks, Vyacheslav Dubeyko.
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c index 8a8893d522ef..a930ddd15681 100644 --- a/fs/hfsplus/extents.c +++ b/fs/hfsplus/extents.c @@ -237,7 +237,9 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock, ablock = iblock >> sbi->fs_shift; if (iblock >= hip->fs_blocks) { - if (iblock > hip->fs_blocks || !create) + if (!create) + return 0; + if (iblock > hip->fs_blocks) return -EIO; if (ablock >= hip->alloc_blocks) { res = hfsplus_file_extend(inode, false);
Direct writes to empty inodes fail with EIO. The generic direct-io code is in part to blame (a patch has been submitted as "direct-io: allow direct writes to empty inodes"), but hfsplus is worse affected than the other filesystems because the fallback to buffered I/O doesn't happen. The problem is the return value of hfsplus_get_block() when called with !create. Change it to be more consistent with the other modules. Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> --- fs/hfsplus/extents.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)