diff mbox series

[RFC,v3,07/10] ovl: implement overlayfs' ->write_inode operation

Message ID 20201108140307.1385745-8-cgxu519@mykernel.net (mailing list archive)
State New, archived
Headers show
Series implement containerized syncfs for overlayfs | expand

Commit Message

Chengguang Xu Nov. 8, 2020, 2:03 p.m. UTC 
Implement overlayfs' ->write_inode to sync dirty data
and redirty overlayfs' inode if necessary.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/super.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Jan Kara Nov. 10, 2020, 1:45 p.m. UTC | #1 
On Sun 08-11-20 22:03:04, Chengguang Xu wrote:
> +static int ovl_write_inode(struct inode *inode,
> +			   struct writeback_control *wbc)
> +{
> +	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> +	struct inode *upper = ovl_inode_upper(inode);
> +	unsigned long iflag = 0;
> +	int ret = 0;
> +
> +	if (!upper)
> +		return 0;
> +
> +	if (!ovl_should_sync(ofs))
> +		return 0;
> +
> +	if (upper->i_sb->s_op->write_inode)
> +		ret = upper->i_sb->s_op->write_inode(inode, wbc);
> +
> +	iflag |= upper->i_state & I_DIRTY_ALL;
> +
> +	if (mapping_writably_mapped(upper->i_mapping) ||
> +	    mapping_tagged(upper->i_mapping, PAGECACHE_TAG_WRITEBACK))
> +		iflag |= I_DIRTY_PAGES;
> +
> +	if (iflag)
> +		ovl_mark_inode_dirty(inode);

I think you didn't incorporate feedback we were speaking about in the last
version of the series. May comment in [1] still applies - you can miss
inodes dirtied through mmap when you decide to clean the inode here. So
IMHO you need something like:

	if (inode_is_open_for_write(inode))
		ovl_mark_inode_dirty(inode);

here to keep inode dirty while it is open for write (and not based on upper
inode state which is unreliable).

								Honza

[1] https://lore.kernel.org/linux-fsdevel/20201105140332.GG32718@quack2.suse.cz/

> +
> +	return ret;
> +}
> +
>  static void ovl_evict_inode(struct inode *inode)
>  {
>  	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> @@ -411,6 +440,7 @@ static const struct super_operations ovl_super_operations = {
>  	.destroy_inode	= ovl_destroy_inode,
>  	.drop_inode	= generic_delete_inode,
>  	.evict_inode	= ovl_evict_inode,
> +	.write_inode	= ovl_write_inode,
>  	.put_super	= ovl_put_super,
>  	.sync_fs	= ovl_sync_fs,
>  	.statfs		= ovl_statfs,
> -- 
> 2.26.2
> 
>
Chengguang Xu Nov. 10, 2020, 3:12 p.m. UTC | #2 
---- 在 星期二, 2020-11-10 21:45:51 Jan Kara <jack@suse.cz> 撰写 ----
 > On Sun 08-11-20 22:03:04, Chengguang Xu wrote:
 > > +static int ovl_write_inode(struct inode *inode,
 > > +               struct writeback_control *wbc)
 > > +{
 > > +    struct ovl_fs *ofs = inode->i_sb->s_fs_info;
 > > +    struct inode *upper = ovl_inode_upper(inode);
 > > +    unsigned long iflag = 0;
 > > +    int ret = 0;
 > > +
 > > +    if (!upper)
 > > +        return 0;
 > > +
 > > +    if (!ovl_should_sync(ofs))
 > > +        return 0;
 > > +
 > > +    if (upper->i_sb->s_op->write_inode)
 > > +        ret = upper->i_sb->s_op->write_inode(inode, wbc);
 > > +
 > > +    iflag |= upper->i_state & I_DIRTY_ALL;
 > > +
 > > +    if (mapping_writably_mapped(upper->i_mapping) ||
 > > +        mapping_tagged(upper->i_mapping, PAGECACHE_TAG_WRITEBACK))
 > > +        iflag |= I_DIRTY_PAGES;
 > > +
 > > +    if (iflag)
 > > +        ovl_mark_inode_dirty(inode);
 > 
 > I think you didn't incorporate feedback we were speaking about in the last
 > version of the series. May comment in [1] still applies - you can miss
 > inodes dirtied through mmap when you decide to clean the inode here. So
 > IMHO you need something like:
 > 
 >     if (inode_is_open_for_write(inode))
 >         ovl_mark_inode_dirty(inode);
 > 
 > here to keep inode dirty while it is open for write (and not based on upper
 > inode state which is unreliable).

Hi Jan,

I not only checked upper inode state but also checked upper inode mmap(shared) state
using  mapping_writably_mapped(upper->i_mapping). Maybe it's better to move i_state check
after mmap check but isn't above checks enough for mmapped file? 

Below code is the definition of mmapping_writably_mapped(), I think it will check shared mmap
regardless write or read permission though the function name is quite confusable.

static inline int mapping_writably_mapped(struct address_space *mapping)
{
	return atomic_read(&mapping->i_mmap_writable) > 0;
}



Thanks,
Chengguang


 > 
 >                                 Honza
 > 
 > [1] https://lore.kernel.org/linux-fsdevel/20201105140332.GG32718@quack2.suse.cz/
 > 
 > > +
 > > +    return ret;
 > > +}
 > > +
 > >  static void ovl_evict_inode(struct inode *inode)
 > >  {
 > >      struct ovl_fs *ofs = inode->i_sb->s_fs_info;
 > > @@ -411,6 +440,7 @@ static const struct super_operations ovl_super_operations = {
 > >      .destroy_inode    = ovl_destroy_inode,
 > >      .drop_inode    = generic_delete_inode,
 > >      .evict_inode    = ovl_evict_inode,
 > > +    .write_inode    = ovl_write_inode,
 > >      .put_super    = ovl_put_super,
 > >      .sync_fs    = ovl_sync_fs,
 > >      .statfs        = ovl_statfs,
 > > -- 
 > > 2.26.2
 > > 
 > > 
 > -- 
 > Jan Kara <jack@suse.com>
 > SUSE Labs, CR
 >
Amir Goldstein Nov. 10, 2020, 4:18 p.m. UTC | #3 
On Tue, Nov 10, 2020 at 3:45 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 08-11-20 22:03:04, Chengguang Xu wrote:
> > +static int ovl_write_inode(struct inode *inode,
> > +                        struct writeback_control *wbc)
> > +{
> > +     struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> > +     struct inode *upper = ovl_inode_upper(inode);
> > +     unsigned long iflag = 0;
> > +     int ret = 0;
> > +
> > +     if (!upper)
> > +             return 0;
> > +
> > +     if (!ovl_should_sync(ofs))
> > +             return 0;
> > +
> > +     if (upper->i_sb->s_op->write_inode)
> > +             ret = upper->i_sb->s_op->write_inode(inode, wbc);
> > +
> > +     iflag |= upper->i_state & I_DIRTY_ALL;
> > +
> > +     if (mapping_writably_mapped(upper->i_mapping) ||
> > +         mapping_tagged(upper->i_mapping, PAGECACHE_TAG_WRITEBACK))
> > +             iflag |= I_DIRTY_PAGES;
> > +
> > +     if (iflag)
> > +             ovl_mark_inode_dirty(inode);
>
> I think you didn't incorporate feedback we were speaking about in the last
> version of the series. May comment in [1] still applies - you can miss
> inodes dirtied through mmap when you decide to clean the inode here. So
> IMHO you need something like:
>
>         if (inode_is_open_for_write(inode))
>                 ovl_mark_inode_dirty(inode);
>
> here to keep inode dirty while it is open for write (and not based on upper
> inode state which is unreliable).
>

Just to be clear, as long as the ovl inode is open for write, the upper inode
is also open for write via the realfile reference, but not the other
way around -
after open(); mmap(); close() of the overlay file, the ovl inode is not
open for write, but the upper inode is, because ovl_mmap() maps the
realfile and upper inode without taking any reference on the ovl file/inode.

Hence the check for mapping_writably_mapped(upper->i_mapping)
above.

Thanks,
Amir.
Jan Kara Nov. 11, 2020, 10:54 a.m. UTC | #4 
On Tue 10-11-20 23:12:14, Chengguang Xu wrote:
>  ---- 在 星期二, 2020-11-10 21:45:51 Jan Kara <jack@suse.cz> 撰写 ----
>  > On Sun 08-11-20 22:03:04, Chengguang Xu wrote:
>  > > +static int ovl_write_inode(struct inode *inode,
>  > > +               struct writeback_control *wbc)
>  > > +{
>  > > +    struct ovl_fs *ofs = inode->i_sb->s_fs_info;
>  > > +    struct inode *upper = ovl_inode_upper(inode);
>  > > +    unsigned long iflag = 0;
>  > > +    int ret = 0;
>  > > +
>  > > +    if (!upper)
>  > > +        return 0;
>  > > +
>  > > +    if (!ovl_should_sync(ofs))
>  > > +        return 0;
>  > > +
>  > > +    if (upper->i_sb->s_op->write_inode)
>  > > +        ret = upper->i_sb->s_op->write_inode(inode, wbc);
>  > > +
>  > > +    iflag |= upper->i_state & I_DIRTY_ALL;
>  > > +
>  > > +    if (mapping_writably_mapped(upper->i_mapping) ||
>  > > +        mapping_tagged(upper->i_mapping, PAGECACHE_TAG_WRITEBACK))
>  > > +        iflag |= I_DIRTY_PAGES;
>  > > +
>  > > +    if (iflag)
>  > > +        ovl_mark_inode_dirty(inode);
>  > 
>  > I think you didn't incorporate feedback we were speaking about in the last
>  > version of the series. May comment in [1] still applies - you can miss
>  > inodes dirtied through mmap when you decide to clean the inode here. So
>  > IMHO you need something like:
>  > 
>  >     if (inode_is_open_for_write(inode))
>  >         ovl_mark_inode_dirty(inode);
>  > 
>  > here to keep inode dirty while it is open for write (and not based on upper
>  > inode state which is unreliable).
> 
> Hi Jan,
> 
> I not only checked upper inode state but also checked upper inode
> mmap(shared) state using  mapping_writably_mapped(upper->i_mapping).
> Maybe it's better to move i_state check after mmap check but isn't above
> checks enough for mmapped file? 

Ah, sorry, I'm blind! I missed the mapping_writably_mapped() check. Thanks
for explanation.

								Honza
diff mbox series

Patch

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 694eff76eb09..0ddee18b0bab 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -391,6 +391,35 @@  static int ovl_remount(struct super_block *sb, int *flags, char *data)
 	return ret;
 }
 
+static int ovl_write_inode(struct inode *inode,
+			   struct writeback_control *wbc)
+{
+	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+	struct inode *upper = ovl_inode_upper(inode);
+	unsigned long iflag = 0;
+	int ret = 0;
+
+	if (!upper)
+		return 0;
+
+	if (!ovl_should_sync(ofs))
+		return 0;
+
+	if (upper->i_sb->s_op->write_inode)
+		ret = upper->i_sb->s_op->write_inode(inode, wbc);
+
+	iflag |= upper->i_state & I_DIRTY_ALL;
+
+	if (mapping_writably_mapped(upper->i_mapping) ||
+	    mapping_tagged(upper->i_mapping, PAGECACHE_TAG_WRITEBACK))
+		iflag |= I_DIRTY_PAGES;
+
+	if (iflag)
+		ovl_mark_inode_dirty(inode);
+
+	return ret;
+}
+
 static void ovl_evict_inode(struct inode *inode)
 {
 	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
@@ -411,6 +440,7 @@  static const struct super_operations ovl_super_operations = {
 	.destroy_inode	= ovl_destroy_inode,
 	.drop_inode	= generic_delete_inode,
 	.evict_inode	= ovl_evict_inode,
+	.write_inode	= ovl_write_inode,
 	.put_super	= ovl_put_super,
 	.sync_fs	= ovl_sync_fs,
 	.statfs		= ovl_statfs,