On Wed, Nov 30, 2016 at 3:49 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Wed, Nov 30, 2016 at 4:19 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: >> On Wed, Nov 30, 2016 at 1:58 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >>> >>> In case you are still interested, I rebase concurrent copy up >>> on top of overlay-rorw and pushed to branch #concurrent_copyup >>> on my github, with slightly improved commit message. >>> But I guess this merge window is crowded enough already and >>> I personally much rather see rorw merged than concurrent copy up. >> >> My problem with the concurrent copy up is the complexity of locking >> rules. Why not do some sort of overlay private lock here? >> >> If a mutex is deemed too expensive, then just a bool "copying" in >> ovl_entry and a waitq in ovl_fs in would do. >> > > I am quite certain that I was not able to explain my change properly > and that if you understood it you wouldn't call it complex. > If anything, it simplifies the locking rules. > > Before my change, there is no clear definition of which locks are held > when calling ovl_copy_up(), but for all call sites besides > ovl_open_maybe_copy_up(), the overlay inode is already locked (by vfs). > > My change only aligns the ovl_open_maybe_copy_up() case with the > rest of the code paths by taking the inode lock before calling ovl_copy_up() > and then the locking requirements for entry to ovl_copy_up() are > uniform across the code (and documented). > > So why invent a new private lock when the overlay inode lock is perfectly > valid for the job? > > After we have the inode lock in place, lock_rename() is taken when it is > needed for a specific operation (when either upperdir or workdir need to > be locked), so releasing it for data copy up is just a free gift. > > Hope that I managed to clarify rather then confuse more. It is documented and easy to verify that inode lock is held by the VFS for all but one case of copy_up(). However that one exception is not documented and pretty hard to verify. I.e. if there's a call path that results in d_real() being called with inode lock, it's going to deadlock. And if that call path is hard to trigger, then testing won't reveal this and the bug can remain undetected for a long time. The current situation OTOH is perfectly fine, because copy up does not rely on overlay inode being locked, so it simply does not matter if the caller holds it or not. Thanks, Miklos -- 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
