[PATCH] ovl: fix failure to fsync lower dir

[PATCH] ovl: fix failure to fsync lower dir

[Amir Goldstein]

As a writable mount, it is not expected for overlayfs to return
EINVAL/EROFS for fsync, even if dir/file is not changed.

This commit fixes the case of fsync of directory, which is easier to
address, because overlayfs already implements fsync file operation for
directories.

The problem reported by Raphael is that new PostgreSQL 10.0 with a
database in overlayfs where lower layer in squashfs fails to start.
The failure is due to fsync error, when PostgreSQL does fsync on all
existing db directories on startup and a specific directory exists
lower layer with no changes.

Reported-by: Raphael Hertzog <raphael@ouaza.com>
Cc: <stable@vger.kernel.org> # v3.18
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/readdir.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 1e27b9eb389d..61edcc0b9bd6 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -839,10 +839,14 @@ static int ovl_dir_fsync(struct file *file, loff_t start, loff_t end,
 	struct dentry *dentry = file->f_path.dentry;
 	struct file *realfile = od->realfile;
 
+	/* Nothing to sync for lower */
+	if (!OVL_TYPE_UPPER(ovl_path_type(dentry)))
+		return 0;
+
 	/*
 	 * Need to check if we started out being a lower dir, but got copied up
 	 */
-	if (!od->is_upper && OVL_TYPE_UPPER(ovl_path_type(dentry))) {
+	if (!od->is_upper) {
 		struct inode *inode = file_inode(file);
 
 		realfile = lockless_dereference(od->upperfile);
@@ -867,6 +871,7 @@ static int ovl_dir_fsync(struct file *file, loff_t start, loff_t end,
 			}
 			inode_unlock(inode);
 		}
+		od->is_upper = true;
 	}
 
 	return vfs_fsync_range(realfile, start, end, datasync);
-- 
2.7.4

Re: [PATCH] ovl: fix failure to fsync lower dir

[Raphael Hertzog]

Hi Amir,

Le mercredi 08 novembre 2017, Amir Goldstein a écrit :
> The problem reported by Raphael is that new PostgreSQL 10.0 with a
> database in overlayfs where lower layer in squashfs fails to start.
> The failure is due to fsync error, when PostgreSQL does fsync on all
> existing db directories on startup and a specific directory exists
> lower layer with no changes.

I confirm that this patch is enough to fix my issue.

Tested-by: Raphaël Hertzog <hertzog@debian.org>

I wonder if we can have a similar problem with a file. Is it possible to
fsync a file opened in read-only mode? Even if it's possible, I assume
this is really rare.

And when you open a file with read-write permissions, when does the
copy-up happen? If it's immediately, then fsync on such a file is safe as
well.

Cheers,
-- 
Raphaël Hertzog ◈ Writer/Consultant ◈ Debian Developer

Discover the Debian Administrator's Handbook:
→ https://debian-handbook.info/get/

Re: [PATCH] ovl: fix failure to fsync lower dir

[Amir Goldstein]

On Fri, Nov 10, 2017 at 11:10 AM, Raphael Hertzog <raphael@ouaza.com> wrote:
> Hi Amir,
>
> Le mercredi 08 novembre 2017, Amir Goldstein a écrit :
>> The problem reported by Raphael is that new PostgreSQL 10.0 with a
>> database in overlayfs where lower layer in squashfs fails to start.
>> The failure is due to fsync error, when PostgreSQL does fsync on all
>> existing db directories on startup and a specific directory exists
>> lower layer with no changes.
>
> I confirm that this patch is enough to fix my issue.
>
> Tested-by: Raphaël Hertzog <hertzog@debian.org>


Thanks for testing!


>
> I wonder if we can have a similar problem with a file. Is it possible to
> fsync a file opened in read-only mode? Even if it's possible, I assume
> this is really rare.
>

We do have the same problem with file.
It is not rare at all. the command "sync <file>" will do just that.
Only difference is that the fix is not a one liner and I believe hijacking
file operations for regular file is already on Miklos' plans.

> And when you open a file with read-write permissions, when does the
> copy-up happen? If it's immediately, then fsync on such a file is safe as
> well.
>

Yes, copy up happens on open for write.

Amir.

Re: [PATCH] ovl: fix failure to fsync lower dir

[Amir Goldstein]

On Fri, Nov 10, 2017 at 11:10 AM, Raphael Hertzog <raphael@ouaza.com> wrote:
> Hi Amir,
>
> Le mercredi 08 novembre 2017, Amir Goldstein a écrit :
>> The problem reported by Raphael is that new PostgreSQL 10.0 with a
>> database in overlayfs where lower layer in squashfs fails to start.
>> The failure is due to fsync error, when PostgreSQL does fsync on all
>> existing db directories on startup and a specific directory exists
>> lower layer with no changes.
>
> I confirm that this patch is enough to fix my issue.
>
> Tested-by: Raphaël Hertzog <hertzog@debian.org>
>

Miklos,

Did you miss this one or have any reservations about the patch?

Thanks,
Amir.

Re: [PATCH] ovl: fix failure to fsync lower dir

[Miklos Szeredi]

On Wed, Nov 22, 2017 at 6:44 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Nov 10, 2017 at 11:10 AM, Raphael Hertzog <raphael@ouaza.com> wrote:
>> Hi Amir,
>>
>> Le mercredi 08 novembre 2017, Amir Goldstein a écrit :
>>> The problem reported by Raphael is that new PostgreSQL 10.0 with a
>>> database in overlayfs where lower layer in squashfs fails to start.
>>> The failure is due to fsync error, when PostgreSQL does fsync on all
>>> existing db directories on startup and a specific directory exists
>>> lower layer with no changes.
>>
>> I confirm that this patch is enough to fix my issue.
>>
>> Tested-by: Raphaël Hertzog <hertzog@debian.org>
>>
>
> Miklos,
>
> Did you miss this one or have any reservations about the patch?

The "od->is_upper = true;" seems to be wrong.  With that the second
fsync() on that file will use the lower file (the od->realfile)
instead of the upper file (od->upperfile).

With that line removed, I think it's OK.  Queued for this cycle.

Thanks,
Miklos

Re: [PATCH] ovl: fix failure to fsync lower dir

[Amir Goldstein]

On Wed, Nov 22, 2017 at 11:33 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, Nov 22, 2017 at 6:44 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Nov 10, 2017 at 11:10 AM, Raphael Hertzog <raphael@ouaza.com> wrote:
>>> Hi Amir,
>>>
>>> Le mercredi 08 novembre 2017, Amir Goldstein a écrit :
>>>> The problem reported by Raphael is that new PostgreSQL 10.0 with a
>>>> database in overlayfs where lower layer in squashfs fails to start.
>>>> The failure is due to fsync error, when PostgreSQL does fsync on all
>>>> existing db directories on startup and a specific directory exists
>>>> lower layer with no changes.
>>>
>>> I confirm that this patch is enough to fix my issue.
>>>
>>> Tested-by: Raphaël Hertzog <hertzog@debian.org>
>>>
>>
>> Miklos,
>>
>> Did you miss this one or have any reservations about the patch?
>
> The "od->is_upper = true;" seems to be wrong.  With that the second
> fsync() on that file will use the lower file (the od->realfile)
> instead of the upper file (od->upperfile).
>

Right.. I guess I intended to install od->realfile = realfile and then set
od->is_upper = true;
But this optimization, whether correct or not had nothing to do with the
proposed fix.

Thanks,
Amir.