---- 在 星期三, 2021-04-07 15:52:15 Amir Goldstein <amir73il@xxxxxxxxx> 撰写 ----
> On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
> >
> > Currently truncate operation on the file which only has
> > lower will copy-up whole lower file and calling truncate(2)
> > on upper file. It is not efficient for the case which
> > truncates to much smaller size than lower file. This patch
> > tries to avoid unnecessary data copy and truncate operation
> > after copy-up.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@xxxxxxxxxxxx>
> > ---
> > fs/overlayfs/copy_up.c | 18 +++++++++++-------
> > fs/overlayfs/inode.c | 9 ++++++++-
> > fs/overlayfs/overlayfs.h | 2 +-
> > 3 files changed, 20 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index a1a9a150405a..331cc32eac95 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> > }
> >
> > static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > - int flags)
> > + int flags, loff_t size)
> > {
> > int err;
> > DEFINE_DELAYED_CALL(done);
> > @@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > /* maybe truncate regular file. this has no effect on dirs */
> > if (flags & O_TRUNC)
> > ctx.stat.size = 0;
> > + if (size)
> > + ctx.stat.size = size;
>
> Not sure about this, but *maybe* instead we re-interpret O_TRUNC
> internally as "either O_TRUNC or truncate()" and then:
> if (flags & O_TRUNC)
> ctx.stat.size = size;
>
> It would simplify the logic in ovl_copy_up_with_data().
> If you do that, put a comment to clarify that special meaning.
>
> >
> > if (S_ISLNK(ctx.stat.mode)) {
> > ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
> > @@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > return err;
> > }
> >
> > -static int ovl_copy_up_flags(struct dentry *dentry, int flags)
> > +static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
> > {
> > int err = 0;
> > const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
> > @@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
> > next = parent;
> > }
> >
> > - err = ovl_copy_up_one(parent, next, flags);
> > + err = ovl_copy_up_one(parent, next, flags, size);
> >
> > dput(parent);
> > dput(next);
> > @@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
> > if (ovl_open_need_copy_up(dentry, flags)) {
> > err = ovl_want_write(dentry);
> > if (!err) {
> > - err = ovl_copy_up_flags(dentry, flags);
> > + err = ovl_copy_up_flags(dentry, flags, 0);
> > ovl_drop_write(dentry);
> > }
> > }
> > @@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
> > return err;
> > }
> >
> > -int ovl_copy_up_with_data(struct dentry *dentry)
> > +int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
> > {
> > - return ovl_copy_up_flags(dentry, O_WRONLY);
> > + if (size)
> > + return ovl_copy_up_flags(dentry, O_WRONLY, size);
> > + return ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);
>
> Best get rid of this helper and put this logic in ovl_setattr(). see below.
>
> > }
> >
> > int ovl_copy_up(struct dentry *dentry)
> > {
> > - return ovl_copy_up_flags(dentry, 0);
> > + return ovl_copy_up_flags(dentry, 0, 0);
> > }
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index cf41bcb664bc..92f274844947 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> > if (!full_copy_up)
> > err = ovl_copy_up(dentry);
> > else
> > - err = ovl_copy_up_with_data(dentry);
> > + err = ovl_copy_up_with_data(dentry, attr->ia_size);
>
> You do not know that ia_size is valid here.
I think we don't have to worry about validation of ia_size here,
vfs layer has already done simple check for specified size and upper fs
will return error when we set invalid file size after copy-up. Am I missing
something?
> Instead of using this if/else and full_copy_up var, use vars 'flags'
> and 'size' and call ovl_copy_up_flags().
> Instead of full_copy_up = true, set flags and size.
> Then you may also remove ovl_copy_up_with_data() which has no other
> callers.
>
> > if (!err) {
> > struct inode *winode = NULL;
> >
> > upperdentry = ovl_dentry_upper(dentry);
> >
> > if (attr->ia_valid & ATTR_SIZE) {
> > + if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
> > + inode_lock(upperdentry->d_inode);
> > + ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
> > + inode_unlock(upperdentry->d_inode);
>
> All that this is saving is an extra notify_change() call and I am not sure it is
> worth the special casing.
>
> Also, I think that is a bug and would make xfstest overlay/013 fail.
I ran testcases in overlay directory and didn't find failure related to this change.
However, generic/313 failed unexpectedly, the reason is I used full_copy_up var
wrongly, for the file which has upper still needs to go through notify_change().
By the way, I don't fully understand calling copy-up function(ovl_copy_up() or ovl_copy_up_with_data())
even for the file which has upper. Maybe it's better to optimize this part first in separated patch.
> When lower file is being executed, its true that we copy up anyway
> and that it is safe to do that, but test and applications expect to get
> ETXTBSY error all the same.
Actually we have already do the check and return ETXTBSY error, see below.
err = -ETXTBSY;
if (atomic_read(&realinode->i_writecount) < 0)
goto out_drop_write;
Thanks,
Chengguang
>
> Please run xfstests at least the generic/quick and overlay/quick groups
> with -overlay.
>
> There are a lot of generic and overlay tests that do truncate(size), but it's
> hard to say if test coverage for your change is sufficient, so unless you find
> tests that cover it, you may need to write a specialized overlay truncate test.
>
> Thanks,
> Amir.
>
