[PATCH v3] ovl: Sync upper dirty data when sync overlayfs

[PATCH v3] ovl: Sync upper dirty data when sync overlayfs

[Chengguang Xu]

Executes filesystem sync or umount on overlayfs,
dirty data does not be synced as expected on upper filesystem.
This patch fixes sync filesystem method to keep data consistency
for overlayfs.

Signed-off-by: Chengguang Xu <cgxu@mykernel.net>

---

Changes since v2:
- Fix checkpatch warning.

Changes since v1:
- Call __sync_filesystem() instead of directly calling low level
  syncing functions.

 fs/overlayfs/super.c | 6 ++----
 fs/sync.c            | 3 ++-
 include/linux/fs.h   | 1 +
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f5738e9..736f04d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -231,6 +231,7 @@ static void ovl_put_super(struct super_block *sb)
 	kfree(ufs);
 }
 
+/* Sync real dirty inodes in upper filesystem (if it exists) */
 static int ovl_sync_fs(struct super_block *sb, int wait)
 {
 	struct ovl_fs *ufs = sb->s_fs_info;
@@ -240,12 +241,9 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
 	if (!ufs->upper_mnt)
 		return 0;
 	upper_sb = ufs->upper_mnt->mnt_sb;
-	if (!upper_sb->s_op->sync_fs)
-		return 0;
 
-	/* real inodes have already been synced by sync_filesystem(ovl_sb) */
 	down_read(&upper_sb->s_umount);
-	ret = upper_sb->s_op->sync_fs(upper_sb, wait);
+	ret = __sync_filesystem(upper_sb, wait);
 	up_read(&upper_sb->s_umount);
 	return ret;
 }
diff --git a/fs/sync.c b/fs/sync.c
index 83ac79a..76c913e 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -28,7 +28,7 @@
  * wait == 1 case since in that case write_inode() functions do
  * sync_dirty_buffer() and thus effectively write one block at a time.
  */
-static int __sync_filesystem(struct super_block *sb, int wait)
+int __sync_filesystem(struct super_block *sb, int wait)
 {
 	if (wait)
 		sync_inodes_sb(sb);
@@ -39,6 +39,7 @@ static int __sync_filesystem(struct super_block *sb, int wait)
 		sb->s_op->sync_fs(sb, wait);
 	return __sync_blockdev(sb->s_bdev, wait);
 }
+EXPORT_SYMBOL(__sync_filesystem);
 
 /*
  * Write out and wait upon all dirty data associated with this
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 885266a..99c2dd40 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2463,6 +2463,7 @@ static inline bool sb_is_blkdev_sb(struct super_block *sb)
 	return false;
 }
 #endif
+extern int __sync_filesystem(struct super_block *sb, int wait);
 extern int sync_filesystem(struct super_block *);
 extern const struct file_operations def_blk_fops;
 extern const struct file_operations def_chr_fops;
-- 
1.8.3.1

Re: [PATCH v3] ovl: Sync upper dirty data when sync overlayfs

[Amir Goldstein]

On Tue, Nov 28, 2017 at 2:39 PM, Chengguang Xu <cgxu@mykernel.net> wrote:
> Executes filesystem sync or umount on overlayfs,

When executing...

> dirty data does not be synced as expected on upper filesystem.

...does not get synced...

> This patch fixes sync filesystem method to keep data consistency
> for overlayfs.
>
> Signed-off-by: Chengguang Xu <cgxu@mykernel.net>
>
> ---
>

Chengguang,

This patch looks fine, but it is not based on overlayfs-next nor on master.
I guess by "mainline" you meant latest release or something.
Please rebase V4 on overlayfs-next from Miklos' tree.

I enhanced my origin syncfs test to check for syncing of upper files:
https://github.com/amir73il/overlayfs/blob/master/tests/xfs_syncfs.sh

Without your fix, the upper file delalloc blocks are not flushed after syncfs,
but only after fsync. With your fix delalloc blacks are flushed by syncfs.

Can you take the task to make this simple test into an xfstest?
If you need help finding your way around xfstests, I can help you with that.
You do not need the xfs specific parts of the test, only the generic delalloc
part.

I think it is best to make a generic/* test (i.e. not fs specific), which tests
flushing of delalloc blocks on sync/syncfs/fsync for fs that support delalloc,
but it may also make sense to have an overlay/* specific regression test for
syncfs. We can discuss those details after you post a test.

Thanks,
Amir.

Re: [PATCH v3] ovl: Sync upper dirty data when sync overlayfs

[Chengguang Xu]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]


> 在 2017年11月28日，下午10:58，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Tue, Nov 28, 2017 at 2:39 PM, Chengguang Xu <cgxu@mykernel.net> wrote:
>> Executes filesystem sync or umount on overlayfs,
> 
> When executing...
> 
>> dirty data does not be synced as expected on upper filesystem.
> 
> ...does not get synced...
> 
>> This patch fixes sync filesystem method to keep data consistency
>> for overlayfs.
>> 
>> Signed-off-by: Chengguang Xu <cgxu@mykernel.net>
>> 
>> ---
>> 
> 
> Chengguang,
> 
> This patch looks fine, but it is not based on overlayfs-next nor on master.
> I guess by "mainline" you meant latest release or something.
> Please rebase V4 on overlayfs-next from Miklos' tree.
> 
> I enhanced my origin syncfs test to check for syncing of upper files:
> https://github.com/amir73il/overlayfs/blob/master/tests/xfs_syncfs.sh
> 
> Without your fix, the upper file delalloc blocks are not flushed after syncfs,
> but only after fsync. With your fix delalloc blacks are flushed by syncfs.
> 
> Can you take the task to make this simple test into an xfstest?
> If you need help finding your way around xfstests, I can help you with that.
> You do not need the xfs specific parts of the test, only the generic delalloc
> part.
> 
> I think it is best to make a generic/* test (i.e. not fs specific), which tests
> flushing of delalloc blocks on sync/syncfs/fsync for fs that support delalloc,
> but it may also make sense to have an overlay/* specific regression test for
> syncfs. We can discuss those details after you post a test.
> 

OK, I’ll  make test case for different sync scenarios.


> Thanks,
> Amir.
> 


[-- Attachment #2: Type: text/html, Size: 4335 bytes --]