Current syncfs(2) syscall on overlayfs just calls sync_filesystem()
on upper_sb to synchronize whole dirty inodes in upper filesystem
regardless of the overlay ownership of the inode. In the use case of
container, when multiple containers using the same underlying upper
filesystem, it has some shortcomings as below.
(1) Performance
Synchronization is probably heavy because it actually syncs unnecessary
inodes for target overlayfs.
(2) Interference
Unplanned synchronization will probably impact IO performance of
unrelated container processes on the other overlayfs.
This series try to implement containerized syncfs for overlayfs so that
only sync target dirty upper inodes which are belong to specific overlayfs
instance. By doing this, it is able to reduce cost of synchronization and
will not seriously impact IO performance of unrelated processes.
v1->v2:
- Mark overlayfs' inode dirty itself instead of adding notification
mechanism to vfs inode.
v2->v3:
- Introduce overlayfs' extra syncfs wait list to wait target upper inodes
in ->sync_fs.
v3->v4:
- Using wait_sb_inodes() to wait syncing upper inodes.
- Mark overlay inode dirty only when having upper inode and VM_SHARED
flag in ovl_mmap().
- Check upper i_state after checking upper mmap state
in ovl_write_inode.
v4->v5:
- Add underlying inode dirtiness check after mnt_drop_write().
- Handle both wait/no-wait mode of syncfs(2) in overlayfs' ->sync_fs().
Chengguang Xu (10):
ovl: setup overlayfs' private bdi
ovl: implement ->writepages operation
ovl: implement overlayfs' ->evict_inode operation
ovl: mark overlayfs' inode dirty on modification
ovl: mark overlayfs' inode dirty on shared mmap
ovl: implement overlayfs' ->write_inode operation
ovl: cache dirty overlayfs' inode
fs: export wait_sb_inodes()
fs: introduce new helper sync_fs_and_blockdev()
ovl: implement containerized syncfs for overlayfs
fs/fs-writeback.c | 3 +-
fs/overlayfs/file.c | 6 ++++
fs/overlayfs/inode.c | 14 ++++++++
fs/overlayfs/overlayfs.h | 4 +++
fs/overlayfs/super.c | 69 ++++++++++++++++++++++++++++++++++-----
fs/overlayfs/util.c | 21 ++++++++++++
fs/sync.c | 14 +++++---
include/linux/fs.h | 1 +
include/linux/writeback.h | 1 +
9 files changed, 120 insertions(+), 13 deletions(-)
--
2.27.0
Setup overlayfs' private bdi so that we can collect
overlayfs' own dirty inodes.
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/overlayfs/super.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 178daa5e82c9..51886ba6130a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1980,6 +1980,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
if (!ofs)
goto out;
+ err = super_setup_bdi(sb);
+ if (err)
+ goto out_err;
+
err = -ENOMEM;
ofs->creator_cred = cred = prepare_creds();
if (!cred)
--
2.27.0
Mark overlayfs' inode dirty on modification so that
we can recognize and collect target inodes for syncfs.
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/overlayfs/inode.c | 1 +
fs/overlayfs/overlayfs.h | 4 ++++
fs/overlayfs/util.c | 21 +++++++++++++++++++++
3 files changed, 26 insertions(+)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index d854e59a3710..4a03aceaeedc 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -478,6 +478,7 @@ int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
if (upperpath.dentry) {
touch_atime(&upperpath);
inode->i_atime = d_inode(upperpath.dentry)->i_atime;
+ ovl_mark_inode_dirty(inode);
}
}
return 0;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3894f3347955..5a016baa06dd 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -276,6 +276,7 @@ static inline bool ovl_allow_offline_changes(struct ovl_fs *ofs)
/* util.c */
+void ovl_mark_inode_dirty(struct inode *inode);
int ovl_want_write(struct dentry *dentry);
void ovl_drop_write(struct dentry *dentry);
struct dentry *ovl_workdir(struct dentry *dentry);
@@ -529,6 +530,9 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
to->i_mtime = from->i_mtime;
to->i_ctime = from->i_ctime;
i_size_write(to, i_size_read(from));
+
+ if (ovl_inode_upper(to) && from->i_state & I_DIRTY_ALL)
+ ovl_mark_inode_dirty(to);
}
/* vfs inode flags copied from real to ovl inode */
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f48284a2a896..5441eae2e345 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -25,7 +25,14 @@ int ovl_want_write(struct dentry *dentry)
void ovl_drop_write(struct dentry *dentry)
{
struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct dentry *upper;
+
mnt_drop_write(ovl_upper_mnt(ofs));
+ if (d_inode(dentry)) {
+ upper = ovl_dentry_upper(dentry);
+ if (upper && d_inode(upper) && d_inode(upper)->i_state & I_DIRTY_ALL)
+ ovl_mark_inode_dirty(d_inode(dentry));
+ }
}
struct dentry *ovl_workdir(struct dentry *dentry)
@@ -1060,3 +1067,17 @@ int ovl_sync_status(struct ovl_fs *ofs)
return errseq_check(&mnt->mnt_sb->s_wb_err, ofs->errseq);
}
+
+/*
+ * We intentionally add I_DIRTY_SYNC flag regardless dirty flag
+ * of upper inode so that we have chance to invoke ->write_inode
+ * to re-dirty overlayfs' inode during writeback process.
+ */
+void ovl_mark_inode_dirty(struct inode *inode)
+{
+ struct inode *upper = ovl_inode_upper(inode);
+ unsigned long iflag = I_DIRTY_SYNC;
+
+ iflag |= upper->i_state & I_DIRTY_ALL;
+ __mark_inode_dirty(inode, iflag);
+}
--
2.27.0
In order to wait syncing upper inodes we need to
call wait_sb_inodes() in overlayfs' ->sync_fs.
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/fs-writeback.c | 3 ++-
include/linux/writeback.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 81ec192ce067..0438c911241e 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2505,7 +2505,7 @@ EXPORT_SYMBOL(__mark_inode_dirty);
* completed by the time we have gained the lock and waited for all IO that is
* in progress regardless of the order callers are granted the lock.
*/
-static void wait_sb_inodes(struct super_block *sb)
+void wait_sb_inodes(struct super_block *sb)
{
LIST_HEAD(sync_list);
@@ -2589,6 +2589,7 @@ static void wait_sb_inodes(struct super_block *sb)
rcu_read_unlock();
mutex_unlock(&sb->s_sync_lock);
}
+EXPORT_SYMBOL(wait_sb_inodes);
static void __writeback_inodes_sb_nr(struct super_block *sb, unsigned long nr,
enum wb_reason reason, bool skip_if_busy)
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index d1f65adf6a26..d7aacd0434cf 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -198,6 +198,7 @@ void wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,
enum wb_reason reason);
void inode_wait_for_writeback(struct inode *inode);
void inode_io_list_del(struct inode *inode);
+void wait_sb_inodes(struct super_block *sb);
/* writeback.h requires fs.h; it, too, is not included from here. */
static inline void wait_on_inode(struct inode *inode)
--
2.27.0
Now drop overlayfs' inode will sync dirty data,
so we change to only drop clean inode.
The purpose of doing this is to keep compatible
behavior with before because without this change
dropping overlayfs inode will not trigger syncing
of underlying dirty inode.
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/overlayfs/super.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index cddae3ca2fa5..bf4000eb9be8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -441,11 +441,25 @@ static int ovl_write_inode(struct inode *inode,
return ret;
}
+/*
+ * In iput_final(), clean inode will drop directly and dirty inode will
+ * keep in the cache until write back to sync dirty data then add to lru
+ * list to wait reclaim.
+ */
+static int ovl_drop_inode(struct inode *inode)
+{
+ struct inode *upper = ovl_inode_upper(inode);
+
+ if (!upper || !(inode->i_state & I_DIRTY_ALL))
+ return 1;
+ return generic_drop_inode(inode);
+}
+
static const struct super_operations ovl_super_operations = {
.alloc_inode = ovl_alloc_inode,
.free_inode = ovl_free_inode,
.destroy_inode = ovl_destroy_inode,
- .drop_inode = generic_delete_inode,
+ .drop_inode = ovl_drop_inode,
.evict_inode = ovl_evict_inode,
.write_inode = ovl_write_inode,
.put_super = ovl_put_super,
--
2.27.0
Implement overlayfs' ->writepages operation so that
we can sync dirty data/metadata to upper filesystem.
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/overlayfs/inode.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 832b17589733..d854e59a3710 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -659,9 +659,22 @@ static const struct inode_operations ovl_special_inode_operations = {
.update_time = ovl_update_time,
};
+static int ovl_writepages(struct address_space *mapping,
+ struct writeback_control *wbc)
+{
+ struct inode *inode = mapping->host;
+ struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+ struct inode *upper = ovl_inode_upper(inode);
+
+ if (!ovl_should_sync(ofs))
+ return 0;
+ return filemap_fdatawrite_wbc(upper->i_mapping, wbc);
+}
+
static const struct address_space_operations ovl_aops = {
/* For O_DIRECT dentry_open() checks f_mapping->a_ops->direct_IO */
.direct_IO = noop_direct_IO,
+ .writepages = ovl_writepages,
};
/*
--
2.27.0
Now overlayfs can sync proper dirty inodes during syncfs,
so remove unnecessary sync_filesystem() on upper file
system.
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/overlayfs/super.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index bf4000eb9be8..ef998ada6cb9 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -15,6 +15,7 @@
#include <linux/seq_file.h>
#include <linux/posix_acl_xattr.h>
#include <linux/exportfs.h>
+#include <linux/writeback.h>
#include "overlayfs.h"
MODULE_AUTHOR("Miklos Szeredi <[email protected]>");
@@ -281,18 +282,15 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
/*
* Not called for sync(2) call or an emergency sync (SB_I_SKIP_SYNC).
* All the super blocks will be iterated, including upper_sb.
- *
- * If this is a syncfs(2) call, then we do need to call
- * sync_filesystem() on upper_sb, but enough if we do it when being
- * called with wait == 1.
*/
- if (!wait)
- return 0;
upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
down_read(&upper_sb->s_umount);
- ret = sync_filesystem(upper_sb);
+ if (wait)
+ wait_sb_inodes(upper_sb);
+
+ ret = sync_fs_and_blockdev(upper_sb, wait);
up_read(&upper_sb->s_umount);
return ret;
--
2.27.0
On Thu, 23 Sept 2021 at 15:08, Chengguang Xu <[email protected]> wrote:
>
> Now drop overlayfs' inode will sync dirty data,
> so we change to only drop clean inode.
>
> The purpose of doing this is to keep compatible
> behavior with before because without this change
> dropping overlayfs inode will not trigger syncing
> of underlying dirty inode.
>
> Signed-off-by: Chengguang Xu <[email protected]>
> ---
> fs/overlayfs/super.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index cddae3ca2fa5..bf4000eb9be8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -441,11 +441,25 @@ static int ovl_write_inode(struct inode *inode,
> return ret;
> }
>
> +/*
> + * In iput_final(), clean inode will drop directly and dirty inode will
> + * keep in the cache until write back to sync dirty data then add to lru
> + * list to wait reclaim.
> + */
> +static int ovl_drop_inode(struct inode *inode)
> +{
> + struct inode *upper = ovl_inode_upper(inode);
> +
> + if (!upper || !(inode->i_state & I_DIRTY_ALL))
Could we check upper dirtyness here? That would give a more precise result.
Alternatively don't set .drop_inode (i.e. use generic_drop_inode())
and set I_DONTCACHE on overlay inodes. That would cause the upper
inode to be always written back before eviction.
The latter would result in simpler logic, and I think performance-wise
it wouldn't matter. But I may be missing something.
Thanks,
Miklos
在 2021/10/7 19:09, Miklos Szeredi 写道:
> On Thu, 23 Sept 2021 at 15:08, Chengguang Xu <[email protected]> wrote:
>> Now drop overlayfs' inode will sync dirty data,
>> so we change to only drop clean inode.
>>
>> The purpose of doing this is to keep compatible
>> behavior with before because without this change
>> dropping overlayfs inode will not trigger syncing
>> of underlying dirty inode.
>>
>> Signed-off-by: Chengguang Xu <[email protected]>
>> ---
>> fs/overlayfs/super.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index cddae3ca2fa5..bf4000eb9be8 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -441,11 +441,25 @@ static int ovl_write_inode(struct inode *inode,
>> return ret;
>> }
>>
>> +/*
>> + * In iput_final(), clean inode will drop directly and dirty inode will
>> + * keep in the cache until write back to sync dirty data then add to lru
>> + * list to wait reclaim.
>> + */
>> +static int ovl_drop_inode(struct inode *inode)
>> +{
>> + struct inode *upper = ovl_inode_upper(inode);
>> +
>> + if (!upper || !(inode->i_state & I_DIRTY_ALL))
> Could we check upper dirtyness here? That would give a more precise result.
We keep tracking mmapped-file(shared mode) by explicitely marking
overlay inode dirty,
so if we drop overlay inode by checking upper dirtyness, we may lose
control on those mmapped upper inodes.
>
> Alternatively don't set .drop_inode (i.e. use generic_drop_inode())
> and set I_DONTCACHE on overlay inodes. That would cause the upper
> inode to be always written back before eviction.
>
> The latter would result in simpler logic, and I think performance-wise
> it wouldn't matter. But I may be missing something.
I think we may seperate mmapped-file(shared) inode and other inode by
clear/set I_DONTCACHE flag on overlay inode if you prefer this approach.
Thanks,
Chengguang
On Thu, 7 Oct 2021 at 14:04, Chengguang Xu <[email protected]> wrote:
>
> 在 2021/10/7 19:09, Miklos Szeredi 写道:
> > On Thu, 23 Sept 2021 at 15:08, Chengguang Xu <[email protected]> wrote:
> >> Now drop overlayfs' inode will sync dirty data,
> >> so we change to only drop clean inode.
> >>
> >> The purpose of doing this is to keep compatible
> >> behavior with before because without this change
> >> dropping overlayfs inode will not trigger syncing
> >> of underlying dirty inode.
> >>
> >> Signed-off-by: Chengguang Xu <[email protected]>
> >> ---
> >> fs/overlayfs/super.c | 16 +++++++++++++++-
> >> 1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> >> index cddae3ca2fa5..bf4000eb9be8 100644
> >> --- a/fs/overlayfs/super.c
> >> +++ b/fs/overlayfs/super.c
> >> @@ -441,11 +441,25 @@ static int ovl_write_inode(struct inode *inode,
> >> return ret;
> >> }
> >>
> >> +/*
> >> + * In iput_final(), clean inode will drop directly and dirty inode will
> >> + * keep in the cache until write back to sync dirty data then add to lru
> >> + * list to wait reclaim.
> >> + */
> >> +static int ovl_drop_inode(struct inode *inode)
> >> +{
> >> + struct inode *upper = ovl_inode_upper(inode);
> >> +
> >> + if (!upper || !(inode->i_state & I_DIRTY_ALL))
> > Could we check upper dirtyness here? That would give a more precise result.
>
> We keep tracking mmapped-file(shared mode) by explicitely marking
> overlay inode dirty,
>
> so if we drop overlay inode by checking upper dirtyness, we may lose
> control on those mmapped upper inodes.
That's fine, since there are no more mmaps at this point.
> >
> > Alternatively don't set .drop_inode (i.e. use generic_drop_inode())
> > and set I_DONTCACHE on overlay inodes. That would cause the upper
> > inode to be always written back before eviction.
> >
> > The latter would result in simpler logic, and I think performance-wise
> > it wouldn't matter. But I may be missing something.
>
> I think we may seperate mmapped-file(shared) inode and other inode by
>
> clear/set I_DONTCACHE flag on overlay inode if you prefer this approach.
Same reasoning here: after upper inode is written out, the dirtyness
in the overlay inode doesn't matter since there cannot be any active
mmaps.
Thanks,
Miklos
On Thu, 23 Sept 2021 at 15:08, Chengguang Xu <[email protected]> wrote:
>
> Mark overlayfs' inode dirty on modification so that
> we can recognize and collect target inodes for syncfs.
>
> Signed-off-by: Chengguang Xu <[email protected]>
> ---
> fs/overlayfs/inode.c | 1 +
> fs/overlayfs/overlayfs.h | 4 ++++
> fs/overlayfs/util.c | 21 +++++++++++++++++++++
> 3 files changed, 26 insertions(+)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index d854e59a3710..4a03aceaeedc 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -478,6 +478,7 @@ int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
> if (upperpath.dentry) {
> touch_atime(&upperpath);
> inode->i_atime = d_inode(upperpath.dentry)->i_atime;
> + ovl_mark_inode_dirty(inode);
> }
> }
> return 0;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 3894f3347955..5a016baa06dd 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -276,6 +276,7 @@ static inline bool ovl_allow_offline_changes(struct ovl_fs *ofs)
>
>
> /* util.c */
> +void ovl_mark_inode_dirty(struct inode *inode);
> int ovl_want_write(struct dentry *dentry);
> void ovl_drop_write(struct dentry *dentry);
> struct dentry *ovl_workdir(struct dentry *dentry);
> @@ -529,6 +530,9 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
> to->i_mtime = from->i_mtime;
> to->i_ctime = from->i_ctime;
> i_size_write(to, i_size_read(from));
> +
> + if (ovl_inode_upper(to) && from->i_state & I_DIRTY_ALL)
> + ovl_mark_inode_dirty(to);
I'd be more comfortable with calling ovl_mark_inode_dirty() unconditionally.
Checking if there's an upper seems to make no sense, since we should
only be copying the attributes if something was changed, and then it
is an upper inode.
Checking dirty flags on upper inode actually makes this racy:
- upper inode dirtied through overlayfs
- inode writeback starts (e.g. background writeback) on upper inode
- dirty flags are cleared
- check for dirty flags in upper inode above indicates not dirty,
ovl inode not dirtied
- syncfs called, misses this inode
- inode writeback completed after syncfs
> }
>
> /* vfs inode flags copied from real to ovl inode */
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f48284a2a896..5441eae2e345 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -25,7 +25,14 @@ int ovl_want_write(struct dentry *dentry)
> void ovl_drop_write(struct dentry *dentry)
> {
> struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> + struct dentry *upper;
> +
> mnt_drop_write(ovl_upper_mnt(ofs));
> + if (d_inode(dentry)) {
> + upper = ovl_dentry_upper(dentry);
> + if (upper && d_inode(upper) && d_inode(upper)->i_state & I_DIRTY_ALL)
> + ovl_mark_inode_dirty(d_inode(dentry));
ovl_want_write/ovl_drop_write means modification of the upper
filesystem. It may or may not be the given dentry, so this is not the
right place to clall ovl_mark_inode_dirty IMO. Better check all
instances of these and see if there are cases where ovl_copyattr()
doesn't handle inode dirtying, and do it explicitly there.
> + }
> }
>
> struct dentry *ovl_workdir(struct dentry *dentry)
> @@ -1060,3 +1067,17 @@ int ovl_sync_status(struct ovl_fs *ofs)
>
> return errseq_check(&mnt->mnt_sb->s_wb_err, ofs->errseq);
> }
> +
> +/*
> + * We intentionally add I_DIRTY_SYNC flag regardless dirty flag
> + * of upper inode so that we have chance to invoke ->write_inode
> + * to re-dirty overlayfs' inode during writeback process.
> + */
> +void ovl_mark_inode_dirty(struct inode *inode)
> +{
> + struct inode *upper = ovl_inode_upper(inode);
> + unsigned long iflag = I_DIRTY_SYNC;
> +
> + iflag |= upper->i_state & I_DIRTY_ALL;
> + __mark_inode_dirty(inode, iflag);
> +}
I think ovl_mark_inode_dirty() can just call mark_inode_dirty().
And so that can go in "overlayfs.h" file as static inline.
Thanks,
Miklos