Re: [PATCH 01/11] ovl: Create origin xattr on copy up for all files

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Oct 18, 2017 at 07:09:37AM +0300, Amir Goldstein wrote:
> On Wed, Oct 18, 2017 at 12:05 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Right now my understanding is that origin xattr is created for all copied
> > up files if index=on. And if index=off, then we create it for all type
> > of files except hardlinks (nlink != 1).
> >
> > With metadata only copy up, I will still require origin xattr to copy up
> > data later, so create it even for hardlinks even with index=off. I am
> > hoping it does not break anything because we do OVL_INDEX test before
> > using inode number of origin.
> 
> I still have a nudging buzz that loosing the information about this upper
> being a broken hardlink may come back to bite us some time, but can't
> put my finger on how.

Hi Amir,

Can't we do stat() on ORIGIN and check nlink to figure out if it is a broken
hard link or not. (If it ever becomes a problem in future). IOW, we still
have a way to figure out if this is broken hard link or not. Just that
it involves extra step of doing stat() and its not the presence/absense
of ORIGIN.

> 
> To reduce the level of that buzz, I suggest to set origin for broken hardlink
> only when doing metacopy and then break the origin association when
> removing metacopy xattr.

While removing metacopy xattr, I will not have any idea whether it is
broken hard link or not. I guess I will have to use above trick and do
stat() on ORIGIN and detect broken hard links.

I feel really odd about special casing this broken hard link case and
setting and clearing ORIGIN with metacopy flags. It makes code
twisted and complicated.

Given stat() should allow us to figure out if something is broken hard
link or not, does it make you feel any better about it.

> 
> In any case, this patch needs to update the comment above setting
> origin xattr that claims to break the association with broken hardlink.

Ok, Will do.

Vivek
> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index c441f9387a1b..0a6294ea64d5 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -538,9 +538,6 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
> >             c->stat.nlink > 1)
> >                 indexed = true;
> >
> > -       if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || indexed)
> > -               c->origin = true;
> > -
> >         if (indexed) {
> >                 c->destdir = ovl_indexdir(c->dentry->d_sb);
> >                 err = ovl_get_index_name(c->lowerpath.dentry, &c->destname);
> > @@ -596,6 +593,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> >                 .parent = parent,
> >                 .dentry = dentry,
> >                 .workdir = ovl_workdir(dentry),
> > +               .origin = true,
> >         };
> >
> >         if (WARN_ON(!ctx.workdir))
> > --
> > 2.13.5
> >