On Fri, Mar 30, 2018 at 12:54:14PM +0300, Amir Goldstein wrote:
> On Thu, Mar 29, 2018 at 10:38 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> > If a dentry has copy up origin, we set flag OVL_PATH_ORIGIN. So far
> > this decision was easy that we had to check only for oe->numlower
> > and if it is non-zero, we knew there is copy up origin. (For non-dir
> > we installed origin dentry in lowerstack[0]).
> >
> > But we don't create ORGIN xattr for broken hardlinks (index=off). And
> > with metacopy feature it is possible that we will still install
> > lowerstack[0]. But that's lower data dentry of metacopy upper of broken
> > hardlink and not ORIGIN XATTR is not set.
> >
> > So two differentiate between two cases, do not set OVL_PATH_ORIGIN if
> > we have a broken hardlink.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > ---
> > fs/overlayfs/util.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 29f7336ade88..961d65bd25c9 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -117,7 +117,14 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
> > * Non-dir dentry can hold lower dentry of its copy up origin.
>
> This comment needs updating with metacopy.
>
> > */
> > if (oe->numlower) {
> > - type |= __OVL_PATH_ORIGIN;
> > + /*
> > + * ORIGIN is created for everyting except broken
> > + * hardlinks
> > + */
> > + if (!(d_inode(dentry)->i_nlink > 1 &&
> > + !ovl_test_flag(OVL_INDEX, d_inode(dentry))))
> > + type |= __OVL_PATH_ORIGIN;
> > +
>
> I don't like relying on overlay nlink. it is not reliable.
> And I think you missed the directory case.
Directory should be fine because for directories ->i_nlink == 1 and this
condition will become true.
> The information you need was available during lookup
> and we did not keep it (was lower verified by origin fh).
> Most likely what we need to do it store OVL_ORIGIN inode flag
> during lookup and then use ovl_test_flag(OVL_ORIGIN)
> in place of OVL_TYPE_ORIGIN(type).
>
> If I am not mistaken, you could set flag OVL_ORIGIN in
> ovl_get_inode() IFF (upperdentry && bylower), but to be honest
> the rules became so complicated that I can't say for sure.
> At least I concentrated all the rules in one helper ovl_hash_bylower(),
> so I hope that helps.
Ok, I can put another ovl-inode flag say OVL_ORIGIN. But that also means
that I need to set the flag during copy up. And that means it brings
back ordering requirements for lockless access. For example, in
ovl_getattr(), if we replace OVL_TYPE_ORIGIN() with ovl_test_flag(ORIGIN),
without explicit barriers, what's the guarantee a user will see OVL_ORIGIN
flag yet. In fact it can happen that user might see upperdentry but not
OVL_ORIGIN.
In fact I see OVL_INDEX being used in ovl_getattr(). If a parallel copy
up took place it is possible that ovl_getattr() saw upperdentry but not
the OVL_INDEX. And then nlink count will be off. IOW, what barrier/lock
guarantees that this does not happen.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
