Re: [PATCH v10 20/21] ext4: Add DAX functionality

[Boaz Harrosh]

On 09/11/2014 07:38 AM, Dave Chinner wrote:
<>
> 
> 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?

(You have commented on the ext4 patch but this is already earlier in ext2
 so I did not see it, sorry. "If you explain slow I finally get it ;-)" )

> Cheers,
> Dave.

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.

Thanks for explaining
Boaz


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/