On Tue, Feb 5, 2019 at 2:03 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Tue, Feb 5, 2019 at 9:42 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > > > > > > > EIO on clone/dedupe: > > > > > generic/303 generic/304 > > > > > > > > > > Wrong st_blocks/fiemap: > > > > > generic/353 generic/392 generic/422 > > > > > > > > Hmm, generic/353 passes for me... > > > > > > > > > > So for me: > > > running generic/353 after generic/303 it fails > > > and running it again it passes. > > > > > > > The failed line in 303,304 is: > > > > xfs_io -c "pread -v -q 9223372036854775806 1" /mnt/test/foo > > pread: Input/output error > > > > And failure seems not related to clone/dedupe at all. > > Easily reproduced on any file (i.e. touch /mnt/test/foo), > > so its probably just an arithmetic len/offset bug. > > > > Yes, pos+PAGE_SIZE < 0 (overrun). > > Pushed a fix to truncate upper read to i_size. > It's more general although maybe non needed > for the common case, where pos+PAGE_SIZE > 0. Yes, we can allow tail page DIO reads, with the exception of the page at LLONG_MAX. > Interesting side effect. Now generic/353 fails always (or more often). > It appears as though a 64K reflinked file is broken to 2 extents > while the test expects it to have a single extent. > > I am not sure why the test makes this expectation, maybe because > it was very probable for buffered write and made it easier to write the test. > The fact is that if we issue buffered write to upper file, the test passes > as expected, but when using direct IO to upper file the test fails. > > I guess that direct IO results in different block reservation than > buffered IO in xfs. So it appears that converting buffered IO to direct IO > on upper file may have some implications. > > Its quite likely that this specific issue will go away with ->writepages(), > so let's wait and see if that is an issue. > The question is whether we want to mess with hidden filesystem > implementation details or we would want to provide a knob for the > user to choose between direct and buffered IO to upper. I don't think anyone outside that testcase would care. I'd just add a knob (and possibly fix the testcase...) > Double page caches can be mitigated by invalidating upper page > cache after writeback. In any case, we need to invalidate upper > page cache after copy up and in other occasions (like when upper > fs has no direct IO support), so the code for using buffered IO to > upper and cleaning upper page cache needs to be implemented anyway. I think the only case where upper doesn't have direct IO support is tmpfs. And in that case we should just turn off overlay pagecache by default, since there's no upside to having a cache at the overlay level. Same probably holds for DAX mounted filesystems as well. Thanks, Miklos
