generic_file_aio_read ?: No locking means DATA CORRUPTION read and write on same 4096 page range

Bug 50981 Theodore Ts'o 6 participants 12 messages Nov 26, 2012 – Nov 27, 2012 2 messages not in the archive

Subsystems (2)

Memory management VFS

lore

Share & tools

lore

Theodore Ts'o

author

2012-11-26 16:45 UTC #2 lore raw

In reply to a message not in the archive

Quoted 5 lines · On Mon, Nov 26, 2012 at 04:33:28PM +0000, [email protected]…

https://bugzilla.kernel.org/show_bug.cgi?id=50981
as this is working properly with XFS, so in ext4/ext3...etc also we shouldn't require synchronization at the Application level,., FS should take care of locking... will we expecting the fix for the same ???

Meetmehiro,

At this point, there seems to be consensus that the kernel should take care of the locking, and that this is not something that needs be a worry for the application. Whether this should be done in the file system layer or in the mm layer is the current question at hand --- since this is a bug that also affects btrfs and other non-XFS file systems.

So the question is whether every file system which supports AIO should add its own locking, or whether it should be done at the mm layer, and at which point the lock in the XFS layer could be removed as no longer necessary.

I've added linux-mm and linux-fsdevel to make sure all of the relevant kernel developers are aware of this question/issue.

Regards,

- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hiro Lalwani

2012-11-26 18:59 UTC #3 lore raw

Thanks a lot Theodore Ts'o ...

Thanks to all (kernel,ext4 ..etc. )developer for quick debug and
response...

Quoted 31 lines · Theodore Ts'o

Quoted 3 lines · On Mon, Nov 26, 2012 at 04:33:28PM +0000, [email protected]…
https://bugzilla.kernel.org/show_bug.cgi?id=50981
as this is working properly with XFS, so in ext4/ext3...etc also we
shouldn't
Quoted 1 line
require synchronization at the Application level,., FS should take care
of
Quoted 1 line
locking... will we expecting the fix for the same ???
Meetmehiro,
At this point, there seems to be consensus that the kernel should take care of the locking, and that this is not something that needs be a worry for the application. Whether this should be done in the file system layer or in the mm layer is the current question at hand --- since this is a bug that also affects btrfs and other non-XFS file systems.
So the question is whether every file system which supports AIO should add its own locking, or whether it should be done at the mm layer, and at which point the lock in the XFS layer could be removed as no longer necessary.
I've added linux-mm and linux-fsdevel to make sure all of the relevant kernel developers are aware of this question/issue.
Regards,
- Ted

Signature · 2 lines

thanks & regards
Hiro Lalwani

Hugh Dickins

2012-11-26 20:05 UTC #4 lore raw

In reply to Theodore Ts'o

Quoted 7 lines · Theodore Ts'o

Quoted 5 lines · On Mon, Nov 26, 2012 at 04:33:28PM +0000, [email protected]…
https://bugzilla.kernel.org/show_bug.cgi?id=50981
as this is working properly with XFS, so in ext4/ext3...etc also we shouldn't require synchronization at the Application level,., FS should take care of locking... will we expecting the fix for the same ???

Quoted 5 lines

Meetmehiro,
At this point, there seems to be consensus that the kernel should take care of the locking, and that this is not something that needs be a worry for the application.

Gosh, that's a very sudden new consensus. The consensus over the past ten or twenty years has been that the Linux kernel enforce locking for consistent atomic writes, but skip that overhead on reads - hasn't it?

Quoted 5 lines

Whether this should be done in the file
system layer or in the mm layer is the current question at hand --- since this is a bug that also affects btrfs and other non-XFS file systems.

Quoted 7 lines

So the question is whether every file system which supports AIO should add its own locking, or whether it should be done at the mm layer, and at which point the lock in the XFS layer could be removed as no longer necessary.
I've added linux-mm and linux-fsdevel to make sure all of the relevant kernel developers are aware of this question/issue.

Thanks, that's helpful; but I think linux-mm people would want to defer to linux-fsdevel maintainers on this: mm/filemap.c happens to be in mm/, but a fundamental change to VFS locking philosophy is not mm's call.

I don't see that page locking would have anything to do with it: if we are going to start guaranteeing reads atomic against concurrent writes, then surely it's the size requested by the user to be guaranteed, spanning however many pages and fs-blocks: i_mutex, or a more efficiently crafted alternative.

Hugh

Christoph Hellwig

2012-11-26 20:13 UTC #5 lore raw

Quoted 3 lines · Hugh Dickins

Gosh, that's a very sudden new consensus. The consensus over the past ten or twenty years has been that the Linux kernel enforce locking for consistent atomic writes, but skip that overhead on reads - hasn't it?

I'm not sure there was much of a consensus ever. We XFS people always ttried to push everyone down the strict rule, but there was enough pushback that it didn't actually happen.

Quoted 9 lines

Thanks, that's helpful; but I think linux-mm people would want to defer to linux-fsdevel maintainers on this: mm/filemap.c happens to be in mm/, but a fundamental change to VFS locking philosophy is not mm's call.
I don't see that page locking would have anything to do with it: if we are going to start guaranteeing reads atomic against concurrent writes, then surely it's the size requested by the user to be guaranteed, spanning however many pages and fs-blocks: i_mutex, or a more efficiently crafted alternative.

What XFS does is simply replace (or rather augment currently) i_mutex with a rw_semaphore (i_iolock in XFS) which is used the following way:

exclusive:
- buffer writes
- pagecache flushing before direct I/O (then downgraded)
- appending direct I/O writes
- less than blocksize granularity direct I/O

shared:
- everything else (buffered reads, "normal" direct I/O)

Doing this in the highest levels of the generic_file_ code would be trivial, and would allow us to get rid of a fair chunk of wrappers in XFS.

Note that we've been thinking about replacing this lock with a range lock, but this will require more research.

Zach Brown

2012-11-26 20:15 UTC #13 lore raw

In reply to Hugh Dickins

Quoted 10 lines · Hugh Dickins

Quoted 9 lines · Theodore Ts'o
Quoted 5 lines · On Mon, Nov 26, 2012 at 04:33:28PM +0000, [email protected]…
https://bugzilla.kernel.org/show_bug.cgi?id=50981
as this is working properly with XFS, so in ext4/ext3...etc also we shouldn't require synchronization at the Application level,., FS should take care of locking... will we expecting the fix for the same ???
Meetmehiro,

Quoted 7 lines

Quoted 3 lines
At this point, there seems to be consensus that the kernel should take care of the locking, and that this is not something that needs be a worry for the application.
Gosh, that's a very sudden new consensus. The consensus over the past ten or twenty years has been that the Linux kernel enforce locking for consistent atomic writes, but skip that overhead on reads - hasn't it?

I was wondering exactly the same thing.

Quoted 4 lines

Quoted 4 lines
So the question is whether every file system which supports AIO should add its own locking, or whether it should be done at the mm layer, and at which point the lock in the XFS layer could be removed as no longer necessary.

(This has nothing to do with AIO. Buffered reads have been copied from unlocked pages.. basically forever, right?)

Quoted 9 lines

Thanks, that's helpful; but I think linux-mm people would want to defer to linux-fsdevel maintainers on this: mm/filemap.c happens to be in mm/, but a fundamental change to VFS locking philosophy is not mm's call.
I don't see that page locking would have anything to do with it: if we are going to start guaranteeing reads atomic against concurrent writes, then surely it's the size requested by the user to be guaranteed, spanning however many pages and fs-blocks: i_mutex, or a more efficiently crafted alternative.

Agreed. While this little racing test might be fixed, those baked in page_size == 4k == atomic granularity assumptions are pretty sketchy.

So we're talking about holding multiple page locks? Or i_mutex? Or some fancy range locking?

There's consensus on serializing overlapping buffered reads and writes?

- z
*readying the read(, mmap(), ) fault deadlock toy*

Dave Chinner

2012-11-26 21:28 UTC #6 lore raw

In reply to Christoph Hellwig

Quoted 19 lines · Christoph Hellwig

Quoted 3 lines · Hugh Dickins
Gosh, that's a very sudden new consensus. The consensus over the past ten or twenty years has been that the Linux kernel enforce locking for consistent atomic writes, but skip that overhead on reads - hasn't it?
I'm not sure there was much of a consensus ever. We XFS people always ttried to push everyone down the strict rule, but there was enough pushback that it didn't actually happen.
Quoted 9 lines
Thanks, that's helpful; but I think linux-mm people would want to defer to linux-fsdevel maintainers on this: mm/filemap.c happens to be in mm/, but a fundamental change to VFS locking philosophy is not mm's call.
I don't see that page locking would have anything to do with it: if we are going to start guaranteeing reads atomic against concurrent writes, then surely it's the size requested by the user to be guaranteed, spanning however many pages and fs-blocks: i_mutex, or a more efficiently crafted alternative.

Quoted 8 lines

What XFS does is simply replace (or rather augment currently) i_mutex with a rw_semaphore (i_iolock in XFS) which is used the following way:
exclusive:
- buffer writes
- pagecache flushing before direct I/O (then downgraded)
- appending direct I/O writes
- less than blocksize granularity direct I/O

- splice write

Also, direct extent manipulations that are outside the IO path such as:
- truncate
- preallocation
- hole punching

use the XFS_IOLOCK_EXCL to provide exclusion against new IO starting while such an operation is in progress.

Quoted 6 lines

shared:
- everything else (buffered reads, "normal" direct I/O)
Doing this in the highest levels of the generic_file_ code would be trivial, and would allow us to get rid of a fair chunk of wrappers in XFS.

We still need the iolock deep in the guts of the filesystem, though.

I suspect that if we are going to change the VFS locking, then we should seriously consider allowing the filesystem to provide it's own locking implementation and the VFS just pass the type of lock required. Otherwise we are still going to need all the locking within the filesystem to serialise all the core pieces that the VFS locking doesn't serialise (e.g. EOF truncation on close/evict, extent swaps for online defrag, etc).

Quoted 2 lines

Note that we've been thinking about replacing this lock with a range lock, but this will require more research.

I'd say we need a working implementation in a filesystem before even considering a VFS implementation...

Cheers,

Dave.

Signature · 8 lines

Dave Chinner
[email protected]

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Hellwig

2012-11-26 21:39 UTC #7 lore raw

Quoted 1 line · Dave Chinner

We still need the iolock deep in the guts of the filesystem, though.

I don't think we do. The only thing that comes close to it is xfs_swap_extents passing the XFS_IOLOCK_EXCL to xfs_trans_ijoin so that the transaction commit automatically unlocks it, but that can be trivially replaced with a manual unlock.

Quoted 7 lines

I suspect that if we are going to change the VFS locking, then we should seriously consider allowing the filesystem to provide it's own locking implementation and the VFS just pass the type of lock required. Otherwise we are still going to need all the locking within the filesystem to serialise all the core pieces that the VFS locking doesn't serialise (e.g. EOF truncation on close/evict, extent swaps for online defrag, etc).

The VFS currently doesn't hardcode i_mutex for any data plane operations, only a few generic helpers do it, most notably generic_file_aio_write (which can be bypassed by using a slightly lower level variant) and __blockdev_direct_IO when used in DIO_LOCKING mode.

Theodore Ts'o

author

2012-11-26 21:49 UTC #8 lore raw

In reply to Christoph Hellwig

Quoted 8 lines · Christoph Hellwig

Quoted 3 lines · Hugh Dickins
Gosh, that's a very sudden new consensus. The consensus over the past ten or twenty years has been that the Linux kernel enforce locking for consistent atomic writes, but skip that overhead on reads - hasn't it?
I'm not sure there was much of a consensus ever. We XFS people always ttried to push everyone down the strict rule, but there was enough pushback that it didn't actually happen.

Christoph, can you give some kind of estimate for the overhead that adding this locking in XFS actually costs in practice? And does XFS provide any kind of consistency guarantees if the reads/write overlap spans multiple pages? I assume the answer to that is no, correct?

Thanks,

- Ted

Christoph Hellwig

2012-11-26 22:09 UTC #9 lore raw

Quoted 2 lines · Theodore Ts'o

Christoph, can you give some kind of estimate for the overhead that adding this locking in XFS actually costs in practice?

I don't know any real life measurements, but in terms of implementation the over head is:

a) taking a the rw_semaphore in shared mode for every buffered read
b) taking the slightly slower exclusive rw_semaphore for buffered writes instead of the plain mutex

On the other hand it significantly simplifies the locking for direct I/O and allows parallel direct I/O writers.

Quoted 3 lines

And does XFS
provide any kind of consistency guarantees if the reads/write overlap spans multiple pages? I assume the answer to that is no, correct?

The answer is yes as the lock is taken globally on the inode.

Dave Chinner

2012-11-26 22:17 UTC #12 lore raw

In reply to Theodore Ts'o

Quoted 6 lines · Theodore Ts'o

Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Hugh Dickins
Gosh, that's a very sudden new consensus. The consensus over the past ten or twenty years has been that the Linux kernel enforce locking for consistent atomic writes, but skip that overhead on reads - hasn't it?

Quoted 6 lines

Quoted 3 lines
I'm not sure there was much of a consensus ever. We XFS people always ttried to push everyone down the strict rule, but there was enough pushback that it didn't actually happen.
Christoph, can you give some kind of estimate for the overhead that adding this locking in XFS actually costs in practice?

It doesn't show up any significant numbers in profiles, if that is what you are asking.

I've tested over random 4k reads and writes at over 2 million IOPS to a single file using concurrent direct IO, so the non-exclusive locking overhead is pretty minimal. If the workload is modified slightly to used buffered writes instead of direct IO writes and so triggering shared/exclusive lock contention, then the same workload tends to get limited at around 250,000 IOPS per file. That's a direct result of the exclusive locking limiting the workload to what a single CPU can sustain (i.e difference between 8p @ 250-300k iops vs 1p @ 250k iops on the exclusive locking workload).

Quoted 3 lines

And does XFS
provide any kind of consistency guarantees if the reads/write overlap spans multiple pages? I assume the answer to that is no, correct?

A buffered write is locked exclusive for the entire of the write. That includes multiple page writes as the locking is outside of the begin_write/end_write per-page iteration. Hence the atomicity of the entire buffered write against both buffered read and direct IO is guaranteed.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Theodore Ts'o

author

2012-11-27 01:32 UTC #10 lore raw

In reply to Christoph Hellwig

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Theodore Ts'o
Christoph, can you give some kind of estimate for the overhead that adding this locking in XFS actually costs in practice?
I don't know any real life measurements, but in terms of implementation the over head is:

Quoted 6 lines

a) taking a the rw_semaphore in shared mode for every buffered read
b) taking the slightly slower exclusive rw_semaphore for buffered writes instead of the plain mutex
On the other hand it significantly simplifies the locking for direct I/O and allows parallel direct I/O writers.

I should probably just look at the XFS code, but.... if you're taking an exclusve lock for buffered writes, won't this impact the performance of buffered writes happening in parallel on different CPU's?

- Ted

Dave Chinner

2012-11-27 04:27 UTC #11 lore raw

Quoted 12 lines · Theodore Ts'o

Quoted 11 lines · Christoph Hellwig
Quoted 2 lines · Theodore Ts'o
Christoph, can you give some kind of estimate for the overhead that adding this locking in XFS actually costs in practice?
I don't know any real life measurements, but in terms of implementation the over head is:
a) taking a the rw_semaphore in shared mode for every buffered read
b) taking the slightly slower exclusive rw_semaphore for buffered writes instead of the plain mutex

Quoted 7 lines

Quoted 2 lines
On the other hand it significantly simplifies the locking for direct I/O and allows parallel direct I/O writers.
I should probably just look at the XFS code, but.... if you're taking an exclusve lock for buffered writes, won't this impact the performance of buffered writes happening in parallel on different CPU's?

Indeed it does - see my previous email. But it's no worse than generic_file_aio_write() that takes i_mutex across buffered writes, which is what most filesystems currently do. And FWIW, we also take the i_mutex outside the i_iolock for the buffered write case because generic_file_buffered_write() is documented to require it held. See xfs_rw_ilock() and friends for locking order semantics...

FWIW, this buffered write exclusion is why we have been considering replacing the rwsem with a shared/exclusive range lock - so we can do concurrent non-overlapping reads and writes (for both direct IO and buffered IO) without compromising the POSIX atomic write guarantee (i.e. that a read will see the entire write or none of it). Range locking will allow us to do that for both buffered and direct IO...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]