On Fri, Nov 11, 2016 at 5:40 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > On Fri, Nov 11, 2016 at 04:43:57PM +0200, Amir Goldstein wrote: > >> > Surely, the copying of data itself is outside of that lock, isn't it? >> >> Mmmmmm, no it isn't, but I am going to make it right. > > Well, if you really have the copyup itself done under lock_rename(), you > are looking at potentially copying gigabytes of data from r/o layer to > workdir under fs-wide lock. _That_ is really asking for contention from > hell, and for no good reason. Renames/whiteout creations/removals are > trivial noise compared to that. I'm somewhat surprised, TBH - I thought > the copyup itself is done outside of that thing. Are you sure it really > isn't? 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()? Do you think we should do the same for the remove/create functions or leave the lock_rename() over whiteout+rename+cleanup as it is? 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
