unionfs: Support renaming symbolic links

[des]

Summary

This adds support for renaming a symbolic link found on the lower fs,
which necessitates copying it to the upper fs, as well as basic tests.

MFC after: 1 week
Sponsored by: Klara, Inc.
Sponsored by: NetApp, Inc.

[des]

comment the tests

[olce]

Code looks correct (more precisely, not worse than the existing), but some effort to factor out at least part of the preambles of both the new unionfs_copylink() and the existing unionfs_copyfile(), and both of the new unionfs_vn_symlink_on_upper() and the existing unionfs_vn_create_on_upper(), respectively, would be appreciated.

sys/fs/unionfs/union_subr.c

1651

VOP_READLINK() could be called directly on lvp instead.

[des]

readlink directly from lower vnode

[olce]

Modulo code duplication, that's fine. And thanks for adding a test.

[kib]

sys/fs/unionfs/union_subr.c

1553

If you are going to panic, why not do it before VOP_GETATTR() ? And why it panic and not KASSERT()?

1561

Was there a reason to not use NDINIT()?

1618

Aren't these MNT_RDONLY checks redundant? They should be handled by lookup already.

1656

The kern_renameat() code already gets the busy reference on the fs, so why get it once more there? I do not see that it could be unref-ed before this point. Note that recursing on busy ref is unsafe: if unmount comes in between two refs, we deadlock.

[jah]

sys/fs/unionfs/union_subr.c

1656

I suspect all these things you've noted were just taken from the existing behavior of unionfs_copyfile(), which also means they apply equally to that function and I wouldn't ask @des to fix them here.

For the vn_start_write() issue in particular, I suspect there's at least some reason for that to be in unionfs_copyfile(), namely that (AFAIK) some cases in which unionfs_copyfile() is called, e.g. from unionfs_open(), don't already hold the busy ref on the mount from the calling context.

Would it be better for unionfs_copyfile() to use vn_start_write(..., V_NOWAIT) and return ERELOOKUP if it fails? That may of course still produce spurious failure on attempted unmount if the caller doesn't handle retry on ERELOOKUP.

[kib]

sys/fs/unionfs/union_subr.c

1656

Simply using V_NOWAIT with ERELOOKUP would sometimes create a busy loop. See how M_NOWAIT is used in other places: we must drop all other resources, then do start_write(V_WAIT), vfs_finished_write(), then indeed return ERELOOKUP.

[jah]

sys/fs/unionfs/union_subr.c

1656

Wouldn't part of the problem there be "dropping all other references"? Within the guts of unionfs, I don't think we have a good way to know whether the caller has already issued vn_start_write() and in any case trying to manage the caller references from there would be fragile.

It would probably be better yet to do thing I put int the comment (which I think @olce also called out in his design doc) and replace the guts of unionfs_copyfile() with vn_generic_copy_file_range(), although it looks as though that function would still have an issue with potentially recursing on vn_start_write().