On Thu, Apr 6, 2017 at 6:20 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > On Thu, Mar 30, 2017 at 1:34 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> On Wed, Mar 29, 2017 at 5:36 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >>> Mikos, >>> >>> This patch set implement the 'simple' solution we discussed on LSF. >>> For the special case of all overlays on the same fs with clone support, >>> files are copied up on open for read (as O_TMPFILE) and linked to >>> upperdir on first open for write. >>> >>> - Patches 1-2 are the refactoring I sent you earlier. They are not >>> strictly needed for the consistent_fd feature - up to you. >>> - Patches 3-4 test 'samefs' and 'cloneup' properties of underlying fs. >>> - Patch 5 copies up open for read (for the special case). >>> - Patch 6 defers linking the tmpfile to first open for write. >>> >>> Some of the design choices for patch 6 are questionable: >>> the storing of tempfile in ovl_dentry_update(), >>> temp dentry is freed only on overlay dentry release. >>> awaiting your feedback about those choises. > > My gut feel is that we should throw away the tempfile once all files > referring to the dentry are closed. Memory pressure on dcache does > not seem like a good way to control the number inodes allocated on > underlying fs. > :-/ mmm but doing that would make the second open for read overhead so much worse. And not only the open itself, all temp inode pages that the first reader read are going away leaving nothing around for the second reader. > I'm hoping that this is going to be a temporary solution, because I > think it's unreasonably heavyweight compared to the rareness of the > issue it is solving. I think this should be emphasized: this is for > the paranoid and it will cause a performance to degrade and resource > use to increase (although it may be an acceptable amount). > I agree its a big cost to pay when overlayfs is used to share lower with many containers. But maybe not so much for a single "development container" use case. If I am running a docker image and never access the lower except though overlayfs from my single container, then I have one copy of inode in cache one copy of the inode pages in cache. Considering that for sane development I need all my dentries in cache anyway, then I think that the cost of clone when dentry remains in cache is really not that bad and the cost of an extra allocated inode is something that modern fs (xfs, btrfs) can live with. But of course it must be an opt-in feature... > I also have a feeling that we must get the inode number from the lower > file in this case or it's going to break things (e.g. the pattern: > stat(), open(), fstat() verify st_dev/st_ino unchanged). Same goes for > atime, although that's a much smaller issue. > Then how about the demo patch I sent (rocopyup on getattr). It deals with the pattern above, but it breaks the pattern: stat(), drop caches, stat()... I believe that for the known issues with tar the first pattern is more important Do you know which applications would suffer from second pattern? > Yeah, yeah, copy-up has that issue, but lets remember that copy-up is > a rare event compared to open(O_RDONLY). > > And we need to fix the constant inode number issue anyway. > Yes, but do you think we should delay consistent_fd until we have a full solution for constant_ino? Do you think it is acceptable to fix only stat (to return lower ino) and leave readdir d_ino for later? Thanks, 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
