Re: [PATCH v10 20/21] ext4: Add DAX functionality
From: Dave Chinner <david@fromorbit.com>
Date: 2014-09-15 06:15:35
Also in:
linux-fsdevel, lkml
On Sun, Sep 14, 2014 at 03:25:45PM +0300, Boaz Harrosh wrote:
quoted hunk ↗ jump to hunk
On 09/11/2014 07:38 AM, Dave Chinner wrote: <>quoted
And so ext4 is buggy, because what ext4 does .... ... is not a retry - it falls back to a fundamentally different code path. i.e: sys_write() .... new_sync_write ext4_file_write_iter __generic_file_write_iter(O_DIRECT) written = generic_file_direct_write() if (error || complete write) return /* short write! do buffered IO to finish! */ generic_perform_write() loop { ext4_write_begin ext4_write_end } and so we allocate pages in the page cache and do buffered IO into them because DAX doesn't hook ->writebegin/write_end as we are supposed to intercept all buffered IO at a higher level. This causes data corruption when tested at ENOSPC on DAX enabled ext4 filesystems. I think that it's an oversight and hence a bug that needs to be fixed but I'm first asking Willy to see if it was intentional or not because maybe I missed sometihng in the past 4 months since I've paid really close attention to the DAX code. And in saying that, Boaz, I'd suggest you spend some time looking at the history of the DAX patchset. Pay careful note to who came up with the original idea and architecture that led to the IO path you are so stridently defending.....Yes! you are completely right, and I have not seen this bug. The same bug exist with ext2 as well. I think this is a bug in patch: [PATCH v10 07/21] Replace XIP read and write with DAX I/O It needs a:@@ -2584,7 +2584,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) loff_t endbyte; written = generic_file_direct_write(iocb, from, pos); - if (written < 0 || written == count) + if (written < 0 || written == count || IS_DAX(inode)) goto out; /*Or something like that. Is that what you meant?
Well, that's one way of working around the immediate issue, but I don't think it solves the whole problem. e.g. what do you do with the bit of the partial write that failed? We may have allocated space for it but not written data to it, so to simply fail exposes stale data in the file(*). Hence it's not clear to me that simply returning the short write is a valid solution for DAX-enabled filesystems. I think that the above - initially, at least - is much better than falling back to buffered IO but filesystems are going to have to be updated to work correctly without that fallback.
Yes I agree this is a very bad data corruption bug. I also think that the read path should not be allowed to fall back to buffered IO just the same for the same reason. We must not allow any real data in page_cache for a DAX file.
Right, I didn't check the read path for the same issue as XFS won't return a short read on direct IO unless the read spans EOF. And in that case it won't ever do buffered reads. ;) Cheers, Dave. (*) XFS avoids this problem by always using unwritten extents for direct IO allocation, but I'm pretty sure that ext4 doesn't do this. Using unwritten extents means that we don't expose stale data in the event we don't end up writing to the allocated space. -- Dave Chinner david@fromorbit.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>