Re: [PATCH] ovl: set overlayfs inode's a_ops->direct_IO properly

[Chengguang Xu <cgxu519@mykernel.net>]

---- 在 星期四, 2021-09-30 20:55:54 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > On Tue, 28 Sept 2021 at 14:48, Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > Loop device checks the ability of DIRECT-IO by checking
 > > a_ops->direct_IO of inode, in order to avoid this kind of
 > > false detection we set a_ops->direct_IO for overlayfs inode
 > > only when underlying inode really has DIRECT-IO ability.
 > >
 > > Reported-by: Huang Jianan <huangjianan@oppo.com>
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > 
 > Can you please add  Fixes: and  Cc: stable@vger.kernel.org tags?
 > 
 > > ---
 > >  fs/overlayfs/dir.c       |  2 ++
 > >  fs/overlayfs/inode.c     |  4 ++--
 > >  fs/overlayfs/overlayfs.h |  1 +
 > >  fs/overlayfs/util.c      | 14 ++++++++++++++
 > >  4 files changed, 19 insertions(+), 2 deletions(-)
 > >
 > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
 > > index 1fefb2b8960e..32a60f9e3f9e 100644
 > > --- a/fs/overlayfs/dir.c
 > > +++ b/fs/overlayfs/dir.c
 > > @@ -648,6 +648,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
 > >         /* Did we end up using the preallocated inode? */
 > >         if (inode != d_inode(dentry))
 > >                 iput(inode);
 > > +       else
 > > +               ovl_inode_set_aops(inode);
 > 
 > This is too late, since the dentry was instantiated and can be found
 > through a cached lookup already.
 > 
 > Anyway, I think this can be dropped, since ovl_inode_init() should be
 > called for inodes preallocated by ovl_create_object() as well:
 > inode_insert5() will set I_NEW on the preallocated inode.
 > 
 > It is interesting that ovl_fill_inode() will be called a second time
 > on the preallocated inode.  This is something that should probably be
 > cleaned up, but that's a separate patch.
 > 
 > >
 > >  out_drop_write:
 > >         ovl_drop_write(dentry);
 > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
 > > index 832b17589733..a7a327e4f790 100644
 > > --- a/fs/overlayfs/inode.c
 > > +++ b/fs/overlayfs/inode.c
 > > @@ -659,7 +659,7 @@ static const struct inode_operations ovl_special_inode_operations = {
 > >         .update_time    = ovl_update_time,
 > >  };
 > >
 > > -static const struct address_space_operations ovl_aops = {
 > > +const struct address_space_operations ovl_aops = {
 > >         /* For O_DIRECT dentry_open() checks f_mapping->a_ops->direct_IO */
 > >         .direct_IO              = noop_direct_IO,
 > >  };
 > > @@ -786,6 +786,7 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 > >         ovl_copyattr(realinode, inode);
 > >         ovl_copyflags(realinode, inode);
 > >         ovl_map_ino(inode, ino, fsid);
 > > +       ovl_inode_set_aops(inode);
 > 
 > OVL_UPPERDATA is only set after ovl_get_inode() in all callers.  This
 > needs to be moved into ovl_inode_init() before calling
 > ovl_inode_set_aops() otherwise this won't work correctly for a copied
 > up file.
 > 

Hi Miklos,

I found it's not convenient to move setting OVL_UPPERDATA into ovl_inode_init() because
we should detect different conditions for different callers. How about calling  ovl_inode_set_aops()
after setting OVL_UPPERDATA?


Thanks,
Chengguang