On Fri, Nov 11, 2016 at 7:27 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > On Fri, Nov 11, 2016 at 06:17:07PM +0200, Amir Goldstein wrote: > >> I'm afraid so. >> It seems to me that most of the time, lock_rename() is being used in >> overlayfs for no better alternative to lock 2 directories (work+upper). >> >> My suggestion is a small modification to the overlayfs locking scheme. >> ---Instead of this: >> assert(lock_rename(workdir, upperdir) != NULL)); >> copy_up(src, tmp); >> vfs_rename(tmp, dst); >> unlock_rename(workdir, upperdir); >> >> +++Use this: >> assert(lock_rename(workdir, upperdir) != NULL)); >> mutex_unlock(s_vfs_rename_mutex); >> copy_up(src, tmp); >> inode_unlock(upperdir); >> inode_unlock(workdir); >> assert(lock_rename(workdir, upperdir) != NULL)); >> vfs_rename(tmp, dst); >> unlock_rename(workdir, upperdir); >> >> Miklos, >> >> Do you see any problem with the proposed scheme? >> Anything that can go wrong while releasing the workdir lock before vfs_rename()? > > Huh??? > > ->rename() definitely counts upon parents being locked; please, read the > damn Documentation/filesystems/locking, it's there for a reason. > I know it does. lock_rename() before vfs_rename() re-aquires the parent locks. The reason I am unlocking them is to be able to re-take s_vfs_rename_mutex and 2 parent dirs in correct order. > The real question is why the fsck do you need to lock the workdir for the > duration of copying at all. O_TMPFILE open + writes there doesn't need > lock_rename() *or* parents being locked. You need the parent locked when > you link the sucker in place, but that's it. IDGI... I am starting to understand the reasons of how the current locking scheme for copy up became to be what it is. It stems from the fact the the parent upper dir is used as the synchronization object to avoid double copy up (and my suggested scheme does not preserve that). Then workdir needs to be locked before we link the temp file. And then we cannot lock upperdir->workdir in that order without holding s_vfs_rename_mutex while we do that, so the whole locking became lock_rename() over the whole thing. I can see the following solution: For regular file copy up, don't use workdir at all, use O_TMPFILE instead then only need to take upperdir lock and it can be the synchronization object for the entire copy-up, which also allows for concurrent copy up to different directories. For non-regular file copy up, use workdir as it is used now and take lock_rename() for the entire copy up as it is now. If upper fs does not support O_TMPFILE, we can resort to the contending copy-up. I am working on a RFC patch. Will send it out over weekend. Bare with me guys, I'll get it right eventually... Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
