On Thu, Jul 13, 2017 at 11:19 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> When inodes index feature is enabled, if lower dir does not match the >> origin fh stored in upper dir, or if no lower dir was found by name, >> try to decode the stored origin fh and use the result as the merge dir >> lower dentry. >> >> This change is needed for indexing of merge directories. A merge >> directory is indexed by the file handle of the uppermost lower dir and >> the index will have a reference to the upper dir by file handle. >> Following down from upper by origin file handle ensures the integrity >> of the index for non-stale origin entries. index entries with stale origin >> are going to be cleaned up on mount anyway. > > Not sure I understand. In what case is following down by origin needed? > First, a snip from cover letter, parts of it may belong in this commit message: -------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8 Specifically, patches 9-10 introduce a behavior change in the case of changes to lower layer (rename or delete of lower dir). The new behavior is to verify origin fh and try to decode it if verification fails (overlayfs.txt updated). This change in needed for snapshots and I argue in the commit message of patch 10 why it is needed for indexing of directories. I admit that the argument is not rigorous, but since the behavior of changes to lower layer is documented as "undefined", I recon you don't really mind if we change it for index=on. The rational of making this change in v4.13 as opposed to when directory indexing is implemented, is that index=on already introduces a behavior change related to copying layers, so IMO it is better to introduce all those behavior changes together on index=on. -------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8 My main argument is that 10/10 is needed for snapshots along with patch 9/10 and assuming that you don't object to the change of behavior in 9/10, then perhaps you are ambivalent to this change as well (apparently not). My secondary NFS export related argument goes something like this. <more hand waving, but while writing stuff on the board> An indexed merge dir would look something like: lower/dir[encode_fh] = 0001 upper/dir[encode_fh] = 0002 upper/dir[origin] = 0001 index/0001[upper] = 0002 overlay/dir[encode_fh] = 0001 When encoding the overlay/dir we get a file handle of 0001. When decoding overlay handle 0001, we find an index that points to upper that points to origin 0001, so all is sane. Now, if lower/dir is renamed to dir2 and a new dir is created in its place we get: lower/dir[encode_fh] = 0003 lower/dir2[encode_fh] = 0001 upper/dir[encode_fh] = 0002 upper/dir[origin] = 0001 index/0001[upper_fh] = 0002 overlay/dir[encode_fh] = ??? Now when we encode overlay/dir we either get 0003 (current kernel) or 0002 (patch 9/10) or 0001 (patch 10/10). decoding 0002 will get us the pure upper dir not merged with the new lower/dir which is consistent with what you get in lookup (path 9/10). However! if NFS client has the file handle 0001, from before lower/dir was renamed, then decoding that file handle, will get the same object as when decoding 0002 and that same object will not be re-decoded to 0001, which is another inconsistency. So yes, this certainly qualifies for "If the underlying filesystem is changed, the behavior of the overlay is undefined...", but can if you agree that the cost of patch 10/10 is not high, then it makes the entire lookup/encode/decode flow file handle verified and then we can work with stronger assumptions on overlay NFS handles. </more hand waving> Feeling any more at ease with this argument? Amir. -- 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
