cleanup blockdev_direct_IO locking
Subsystems (3)
PATCH 1/1 cleanup blockdev_direct_IO locking
Currently the locking in blockdev_direct_IO is a mess, we have three different locking types and very confusing checks for some of them. The most complicated one is DIO_OWN_LOCKING for reads, which happens to not actually be used.
This patch gets rid of the DIO_OWN_LOCKING - as mentioned above the read case is unused anyway, and the write side is almost identical to DIO_NO_LOCKING. The difference is that DIO_NO_LOCKING always sets the create argument for the get_blocks callback to zero, but we can easily move that to the actual get_blocks callbacks. There are four users of the DIO_NO_LOCKING mode:
gfs already ignores the create argument and thus is fine with the new version, ocfs2 only errors out if create were ever set, and we can remove this dead code now, the block device code only ever uses create for an error message if we are fully beyond the device which can never happen, and last but not least XFS will need the new behavour for writes.
Once this replace the lock_type variable with a flags one, where no flag means the DIO_NO_LOCKING behaviour and DIO_LOCKING is kept as the first and so far only flag. Also revamp the documentation of the locking scheme to actually make sense.
fs/ocfs2/aops.c +4 -30
@@ -545,6 +545,9 @@ bail:* * called like this: dio->get_blocks(dio->inode, fs_startblk, * fs_count, map_bh, dio->rw == WRITE); +* +* Note that we never bother to allocate blocks here, and thus ignore the +* create argument. */ static int ocfs2_direct_IO_get_blocks (struct inode *inode,sector_t iblock,struct buffer_head *bh_result,int create)
@@ -561,14 +564,6 @@ static int ocfs2_direct_IO_get_blocks(st inode_blocks =ocfs2_blocks_for_bytes (inode->i_sb,i_size_read (inode)); -/* -* Any write past EOF is not allowed because we'd be extending. -*/ -if (create && (iblock + max_blocks) > inode_blocks) { - ret = -EIO ; - goto bail; - } -/* This figures out the size of the next contiguous block, and * our logical offset */ ret = ocfs2_extent_map_get_blocks(inode, iblock, &p_blkno,
@@ -580,15 +575,6 @@ static int ocfs2_direct_IO_get_blocks(st goto bail; } -if (!ocfs2_sparse_alloc (OCFS2_SB (inode->i_sb)) && !p_blkno && create) { -ocfs2_error (inode->i_sb, -"Inode %llu has a hole at block %llu\n" , - (unsigned long long )OCFS2_I (inode)->ip_blkno, - (unsigned long long )iblock); - ret = -EROFS ; - goto bail; - } -/* * get_more_blocks() expects us to describe a hole by clearing * the mapped bit on bh_result().
@@ -597,20 +583,8 @@ static int ocfs2_direct_IO_get_blocks(st*/ if (p_blkno && !(ext_flags &OCFS2_EXT_UNWRITTEN ))map_bh (bh_result, inode->i_sb, p_blkno); -else { -/* -* ocfs2_prepare_inode_for_write() should have caught -* the case where we'd be filling a hole and triggered -* a buffered write instead. -*/ -if (create) { - ret = -EIO ; -mlog_errno (ret); - goto bail; - } - +else clear_buffer_mapped (bh_result); - }/* make sure we don't map more than max_blocks blocks here as that's all the kernel will handle at this point. */
fs/xfs/linux-2.6/xfs_aops.c +7 -13
@@ -1545,19 +1545,13 @@ xfs_vm_direct_IO( bdev =xfs_find_bdev_for_inode (XFS_I (inode)); -if (rw ==WRITE ) { - iocb->private =xfs_alloc_ioend (inode,IOMAP_UNWRITTEN ); - ret =blockdev_direct_IO_own_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - }else { - iocb->private =xfs_alloc_ioend (inode,IOMAP_READ ); - ret =blockdev_direct_IO_no_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - } + iocb->private =xfs_alloc_ioend (inode, rw ==WRITE ? +IOMAP_UNWRITTEN :IOMAP_READ ); + + ret =blockdev_direct_IO_no_locking (rw, iocb, inode, bdev, iov, + offset, nr_segs, + xfs_get_blocks_direct, + xfs_end_io_direct);if (unlikely (ret != -EIOCBQUEUED && iocb->private))xfs_destroy_ioend (iocb->private);
fs/direct-io.c +52 -78
212 lines not on this page · raw message
include/linux/fs.h +1 -12
@@ -2247,8 +2247,6 @@ ssize_t __blockdev_direct_IO(int rw, strenum {DIO_LOCKING =1 ,/* need locking between buffered and direct access */ -DIO_NO_LOCKING ,/* bdev; no locking at all between buffered/direct */ -DIO_OWN_LOCKING ,/* filesystem locks buffered and direct internally */ }; static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,
@@ -2266,16 +2264,7 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_NO_LOCKING ); -} - -static inline ssize_t blockdev_direct_IO_own_locking (int rw,struct kiocb *iocb, -struct inode *inode,struct block_device *bdev,const struct iovec *iov, -loff_t offset,unsigned long nr_segs,get_block_t get_block, -dio_iodone_t end_io) -{ -return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_OWN_LOCKING ); + nr_segs, get_block, end_io,0 ); }#endif
Quoted 16 lines · Christoph Hellwig
Currently the locking in
blockdev_direct_IOis a mess, we have three different locking types and very confusing checks for some of them. The most complicated one isDIO_OWN_LOCKINGfor reads, which happens to not actually be used.This patch gets rid of the
DIO_OWN_LOCKING- as mentioned above the read case is unused anyway, and the write side is almost identical toDIO_NO_LOCKING. The difference is thatDIO_NO_LOCKINGalways sets the create argument for theget_blockscallback to zero, but we can easily move that to the actualget_blockscallbacks. There are four users of theDIO_NO_LOCKINGmode:
gfs already ignores the create argument and thus is fine with the new version, ocfs2 only errors out if create were ever set, and we can remove this dead code now, the block device code only ever uses create for an error message if we are fully beyond the device which can never happen, and last but not least XFS will need the new behavour for writes.
Quoted 6 lines
Once this replace the
lock_typevariable with a flags one, where no flag means theDIO_NO_LOCKINGbehaviour andDIO_LOCKINGis kept as the first and so far only flag. Also revamp the documentation of the locking scheme to actually make sense.Signed-off-by Christoph Hellwig <[email protected]>
I generally like it, though I'd rather DIO_LOCKING didn't imply hole-filling ability; I guess it's related but is it 1:1 ? Maybe we can have a hole-filling-related flag right off the bat. Couple other comments below inline.
<ocfs2 & xfs snipped>
Quoted 6 lines
Index:
linux-2.6/fs/direct-io.c
===================================================================fs/direct-io.c +0
-0@@ -53,13 +53,6 @@*
fs/direct-io.c line 56
Quoted 10 lines
fs/direct-io.c +0
-7* If blkfactor is zero then the user's request was aligned to the filesystem's * blocksize. -* -* lock_type is DIO_LOCKING for regular files on direct-IO-naive filesystems. -* This determines whether we need to do the fancy locking which prevents -* direct-IO from being able to read uninitialised disk blocks. If its zero -* (blockdev) this locking is not done, and if it is DIO_OWN_LOCKING i_mutex is -* not held for the entire direct write (taken briefly, initially, during a -* direct read though, but its never held for the duration of a direct-IO). */
Yay for consolidating the locking comments.
Quoted 21 lines
fs/direct-io.c +3
-2struct dio {@@ -68,7 +61,7 @@ struct dio {struct inode *inode;int rw;loff_t i_size;/* i_size when submitted */ -int lock_type;/* doesn't change */ +int flags;/* doesn't change */ unsigned blkbits;/* doesn't change */ unsigned blkfactor;/* When we're using an alignment which is finer than the filesystem's soft @@ -240,7 +233,8 @@ static int dio_complete(struct dio *dio,if (dio->end_io && dio->result) dio->end_io (dio->iocb, offset, transferred, dio->map_bh.b_private); -if (dio->lock_type ==DIO_LOCKING ) + +if (dio->flags &DIO_LOCKING )/* lockdep: non-owner release */ up_read_non_owner (&dio->inode->i_alloc_sem);
fs/direct-io.c static int get_more_blocks(struct dio *d · line 516
Quoted 8 lines
fs/direct-io.c +5
-0@@ -515,21 +509,22 @@ static int get_more_blocks(struct dio *d map_bh->b_state =0 ; map_bh->b_size = fs_count << dio->inode->i_blkbits; +/* +* For writes inside i_size on a DIO_LOCKING filesystem we +* forbid block creations: only overwrites are permitted. +* For this case we fall back to buffered writes at a higher +* level for inside-i_size block-instantiating writes.
maybe:
+ * when an unmapped buffer is returned.
A note that this determination is made by the caller when it gets back an unmapped bh might help dimwits like me next time I read the code ;)
Quoted 47 lines
fs/direct-io.c +5
-21+* For filesystems with their own locking the decision is +* left to the get_blocks method. +*/ create = dio->rw &WRITE ; -if (dio->lock_type ==DIO_LOCKING ) { +if (dio->flags &DIO_LOCKING ) {if (dio->block_in_file < (i_size_read (dio->inode) >> dio->blkbits)) create =0 ; - }else if (dio->lock_type ==DIO_NO_LOCKING ) { - create =0 ; } -/* -* For writes inside i_size we forbid block creations: only -* overwrites are permitted. We fall back to buffered writes -* at a higher level for inside-i_size block-instantiating -* writes. -*/ ret = (*dio->get_block)(dio->inode, fs_startblk, map_bh, create); }@@ -1042,7 +1037,7 @@ direct_io_worker(int rw, struct kiocb *i* we can let i_mutex go now that its achieved its purpose * of protecting us from looking up uninitialized blocks. */ -if ((rw ==READ ) && (dio->lock_type ==DIO_LOCKING )) +if (rw ==READ && (dio->flags &DIO_LOCKING ))mutex_unlock (&dio->inode->i_mutex);/* @@ -1086,30 +1081,29 @@ direct_io_worker(int rw, struct kiocb *i/* * This is a library function for use by filesystem drivers. -* The locking rules are governed by the dio_lock_type parameter. * -* DIO_NO_LOCKING (no locking, for raw block device access) -* For writes, i_mutex is not held on entry; it is never taken. -* -* DIO_LOCKING (simple locking for regular files) -* For writes we are called under i_mutex and return with i_mutex held, even -* though it is internally dropped. -* For reads, i_mutex is not held on entry, but it is taken and dropped before -* returning. -* -* DIO_OWN_LOCKING (filesystem provides synchronisation and handling of
fs/direct-io.c direct_io_worker(int rw, struct kiocb *i · line 1088
Quoted 10 lines
fs/direct-io.c +4
-6-* uninitialised data, allowing parallel direct readers and writers) -* For writes we are called without i_mutex, return without it, never touch it. -* For reads we are called under i_mutex and return with i_mutex held, even -* though it may be internally dropped. -* -* Additional i_alloc_sem locking requirements described inline below. +* The locking rules are governed by the flags parameter: +* - if the flags value contains DIO_LOCKING we use a fancy locking +* scheme for dumb filesystems. +* For writes this functions is called under i_mutex and return with
^^function ^^returns
fs/direct-io.c direct_io_worker(int rw, struct kiocb *i · line 1089
Quoted 1 line
fs/direct-io.c +1
-0+* i_mutex held, even though it is internally dropped.
Is it? where? Oh I guess in direct_io_worker.
fs/direct-io.c direct_io_worker(int rw, struct kiocb *i · line 1091
Quoted 2 lines
fs/direct-io.c +2
-0+* For reads, i_mutex is not held on entry, but it is taken and dropped +* before returning.
It's actually dropped by direct_io_worker, FWIW.
On my initial read of this stuff it sounds like this locking is done in the function following the comments which confused me.
Quoted 47 lines
fs/direct-io.c +12
-14+* For reads and writes i_alloc_sem is taken in shared mode and released +* on I/O completion (which may happen asynchronously after returning to +* the caller). +* +* - if the flags value does NOT contain DIO_LOCKING we don't use any +* internal locking but rather rely on the filesystem to synchronize +* direct I/O reads/writes versus each other and truncate. +* For reads and writes both i_mutex and i_alloc_sem are not held on +* entry and are never taken. */ ssize_t __blockdev_direct_IO (int rw,struct kiocb *iocb,struct inode *inode,struct block_device *bdev,const struct iovec *iov,loff_t offset,unsigned long nr_segs,get_block_t get_block,dio_iodone_t end_io, -int dio_lock_type) +int flags) {int seg;size_t size;@@ -1120,8 +1114,6 @@ __blockdev_direct_IO(int rw, struct kiocssize_t retval = -EINVAL ;loff_t end = offset;struct dio *dio; -int release_i_mutex =0 ; -int acquire_i_mutex =0 ;if (rw &WRITE ) rw =WRITE_ODIRECT ;@@ -1156,43 +1148,30 @@ __blockdev_direct_IO(int rw, struct kiocif (!dio) goto out; -/* -* For block device access DIO_NO_LOCKING is used, -* neither readers nor writers do any locking at all -* For regular files using DIO_LOCKING, -* readers need to grab i_mutex and i_alloc_sem -* writers need to grab i_alloc_sem only (i_mutex is already held) -* For regular files using DIO_OWN_LOCKING, -* neither readers nor writers take any locks here -*/ - dio->lock_type = dio_lock_type; -if (dio_lock_type !=DIO_NO_LOCKING ) { + dio->flags = flags; +if (dio->flags &DIO_LOCKING ) {/* watch out for a 0 len io from a tricksy fs */ if (rw ==READ && end > offset) {
fs/direct-io.c __blockdev_direct_IO(int rw, struct kioc · line 1158
Quoted 10 lines
fs/direct-io.c +3
-6-struct address_space *mapping; +struct address_space *mapping = + iocb->ki_filp->f_mapping; - mapping = iocb->ki_filp->f_mapping; -if (dio_lock_type !=DIO_OWN_LOCKING ) { -mutex_lock (&inode->i_mutex); - release_i_mutex =1 ; - } +/* will be released by direct_io_worker */
ahah there we go
Quoted 11 lines
fs/direct-io.c +2
-2+mutex_lock (&inode->i_mutex); retval =filemap_write_and_wait_range (mapping, offset, end -1 );if (retval) { +mutex_unlock (&inode->i_mutex);kfree (dio); goto out; } - -if (dio_lock_type ==DIO_OWN_LOCKING ) {
fs/direct-io.c __blockdev_direct_IO(int rw, struct kioc · line 1171
Quoted 10 lines
fs/direct-io.c +2
-6-mutex_unlock (&inode->i_mutex); - acquire_i_mutex =1 ; - } } -if (dio_lock_type ==DIO_LOCKING ) -/* lockdep: not the owner will release it */ -down_read_non_owner (&inode->i_alloc_sem); +/* +* Will be released at I/O completion, possible in a
^^possibly
Quoted 45 lines
fs/direct-io.c +12
-14+* different thread. +*/ +down_read_non_owner (&inode->i_alloc_sem); }/* @@ -1210,24 +1189,19 @@ __blockdev_direct_IO(int rw, struct kioc/* * In case of error extending write may have instantiated a few * blocks outside i_size. Trim these off again for DIO_LOCKING. -* NOTE: DIO_NO_LOCK/DIO_OWN_LOCK callers have to handle this by -* it's own meaner. +* +* NOTE: filesystems with their own locking have to handle this +* on their own. */ -if (unlikely (retval <0 && (rw &WRITE ))) { -loff_t isize =i_size_read (inode); - -if (end > isize && dio_lock_type ==DIO_LOCKING ) -vmtruncate (inode, isize); +if (dio->flags &DIO_LOCKING ) { +if (unlikely ((rw &WRITE ) && retval <0 )) { +loff_t isize =i_size_read (inode); +if (end > isize ) +vmtruncate (inode, isize); + } } -if (rw ==READ && dio_lock_type ==DIO_LOCKING ) - release_i_mutex =0 ; - out: -if (release_i_mutex) -mutex_unlock (&inode->i_mutex); -else if (acquire_i_mutex) -mutex_lock (&inode->i_mutex);return retval; }EXPORT_SYMBOL (__blockdev_direct_IO);Index:
linux-2.6/include/linux/fs.h
===================================================================
include/linux/fs.h ssize_t __blockdev_direct_IO(int rw, str · line 2250
Quoted 6 lines
include/linux/fs.h +0
-2@@ -2247,8 +2247,6 @@ ssize_t __blockdev_direct_IO(int rw, strenum {DIO_LOCKING =1 ,/* need locking between buffered and direct access */ -DIO_NO_LOCKING ,/* bdev; no locking at all between buffered/direct */ -DIO_OWN_LOCKING ,/* filesystem locks buffered and direct internally */ };
If this is meant to be a flag it should probably be
#define DIO_LOCKING 0x1
not an enum - or if it's an enum put it in hex w/ a comment that the values are for flags. enum to me sorta implies that it's sequential but maybe it's just my hangup ;)
Quoted 24 lines
include/linux/fs.h +1
-11static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,@@ -2266,16 +2264,7 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_NO_LOCKING ); -} - -static inline ssize_t blockdev_direct_IO_own_locking (int rw,struct kiocb *iocb, -struct inode *inode,struct block_device *bdev,const struct iovec *iov, -loff_t offset,unsigned long nr_segs,get_block_t get_block, -dio_iodone_t end_io) -{ -return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_OWN_LOCKING ); + nr_segs, get_block, end_io,0 ); }#endif --To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
New version with Eric's comments addresses:
--
Subject: cleanup blockdev_direct_IO locking
From: Christoph Hellwig <[email protected]>
Currently the locking in blockdev_direct_IO is a mess, we have three different locking types and very confusing checks for some of them. The most complicated one is DIO_OWN_LOCKING for reads, which happens to not actually be used.
This patch gets rid of the DIO_OWN_LOCKING - as mentioned above the read case is unused anyway, and the write side is almost identical to DIO_NO_LOCKING. The difference is that DIO_NO_LOCKING always sets the create argument for the get_blocks callback to zero, but we can easily move that to the actual get_blocks callbacks. There are four users of the DIO_NO_LOCKING mode:
gfs already ignores the create argument and thus is fine with the new version, ocfs2 only errors out if create were ever set, and we can remove this dead code now, the block device code only ever uses create for an error message if we are fully beyond the device which can never happen, and last but not least XFS will need the new behavour for writes.
Now we can replace the lock_type variable with a flags one, where no flag means the DIO_NO_LOCKING behaviour and DIO_LOCKING is kept as the first flag. Separate out the check for not allowing to fill holes into a separate flag, although for now both flags always get set at the same time.
Also revamp the documentation of the locking scheme to actually make sense.
fs/ocfs2/aops.c +4 -30
@@ -545,6 +545,9 @@ bail:* * called like this: dio->get_blocks(dio->inode, fs_startblk, * fs_count, map_bh, dio->rw == WRITE); +* +* Note that we never bother to allocate blocks here, and thus ignore the +* create argument. */ static int ocfs2_direct_IO_get_blocks (struct inode *inode,sector_t iblock,struct buffer_head *bh_result,int create)
@@ -561,14 +564,6 @@ static int ocfs2_direct_IO_get_blocks(st inode_blocks =ocfs2_blocks_for_bytes (inode->i_sb,i_size_read (inode)); -/* -* Any write past EOF is not allowed because we'd be extending. -*/ -if (create && (iblock + max_blocks) > inode_blocks) { - ret = -EIO ; - goto bail; - } -/* This figures out the size of the next contiguous block, and * our logical offset */ ret = ocfs2_extent_map_get_blocks(inode, iblock, &p_blkno,
@@ -580,15 +575,6 @@ static int ocfs2_direct_IO_get_blocks(st goto bail; } -if (!ocfs2_sparse_alloc (OCFS2_SB (inode->i_sb)) && !p_blkno && create) { -ocfs2_error (inode->i_sb, -"Inode %llu has a hole at block %llu\n" , - (unsigned long long )OCFS2_I (inode)->ip_blkno, - (unsigned long long )iblock); - ret = -EROFS ; - goto bail; - } -/* * get_more_blocks() expects us to describe a hole by clearing * the mapped bit on bh_result().
@@ -597,20 +583,8 @@ static int ocfs2_direct_IO_get_blocks(st*/ if (p_blkno && !(ext_flags &OCFS2_EXT_UNWRITTEN ))map_bh (bh_result, inode->i_sb, p_blkno); -else { -/* -* ocfs2_prepare_inode_for_write() should have caught -* the case where we'd be filling a hole and triggered -* a buffered write instead. -*/ -if (create) { - ret = -EIO ; -mlog_errno (ret); - goto bail; - } - +else clear_buffer_mapped (bh_result); - }/* make sure we don't map more than max_blocks blocks here as that's all the kernel will handle at this point. */
fs/xfs/linux-2.6/xfs_aops.c +7 -13
@@ -1545,19 +1545,13 @@ xfs_vm_direct_IO( bdev =xfs_find_bdev_for_inode (XFS_I (inode)); -if (rw ==WRITE ) { - iocb->private =xfs_alloc_ioend (inode,IOMAP_UNWRITTEN ); - ret =blockdev_direct_IO_own_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - }else { - iocb->private =xfs_alloc_ioend (inode,IOMAP_READ ); - ret =blockdev_direct_IO_no_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - } + iocb->private =xfs_alloc_ioend (inode, rw ==WRITE ? +IOMAP_UNWRITTEN :IOMAP_READ ); + + ret =blockdev_direct_IO_no_locking (rw, iocb, inode, bdev, iov, + offset, nr_segs, + xfs_get_blocks_direct, + xfs_end_io_direct);if (unlikely (ret != -EIOCBQUEUED && iocb->private))xfs_destroy_ioend (iocb->private);
fs/direct-io.c +53 -78
213 lines not on this page · raw message
include/linux/fs.h +8 -14
@@ -2246,9 +2246,11 @@ ssize_t __blockdev_direct_IO(int rw, str int lock_type);enum { -DIO_LOCKING =1 ,/* need locking between buffered and direct access */ -DIO_NO_LOCKING ,/* bdev; no locking at all between buffered/direct */ -DIO_OWN_LOCKING ,/* filesystem locks buffered and direct internally */ +/* need locking between buffered and direct access */ +DIO_LOCKING =0x01 , + +/* filesystem does not support filling holes */ +DIO_SKIP_HOLES =0x02 , }; static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,
@@ -2257,7 +2259,8 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_LOCKING ); + nr_segs, get_block, end_io, +DIO_LOCKING |DIO_SKIP_HOLES ); } static inline ssize_t blockdev_direct_IO_no_locking(int rw, struct kiocb *iocb,
@@ -2266,16 +2269,7 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_NO_LOCKING ); -} - -static inline ssize_t blockdev_direct_IO_own_locking (int rw,struct kiocb *iocb, -struct inode *inode,struct block_device *bdev,const struct iovec *iov, -loff_t offset,unsigned long nr_segs,get_block_t get_block, -dio_iodone_t end_io) -{ -return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_OWN_LOCKING ); + nr_segs, get_block, end_io,0 ); }#endif
Quoted 172 lines · Christoph Hellwig
New version with Eric's comments addresses:
--
Subject: cleanup
blockdev_direct_IOlocking
From: Christoph Hellwig <[email protected]>Currently the locking in
blockdev_direct_IOis a mess, we have three different locking types and very confusing checks for some of them. The most complicated one isDIO_OWN_LOCKINGfor reads, which happens to not actually be used.This patch gets rid of the
DIO_OWN_LOCKING- as mentioned above the read case is unused anyway, and the write side is almost identical toDIO_NO_LOCKING. The difference is thatDIO_NO_LOCKINGalways sets the create argument for theget_blockscallback to zero, but we can easily move that to the actualget_blockscallbacks. There are four users of theDIO_NO_LOCKINGmode:
gfs already ignores the create argument and thus is fine with the new version, ocfs2 only errors out if create were ever set, and we can remove this dead code now, the block device code only ever uses create for an error message if we are fully beyond the device which can never happen, and last but not least XFS will need the new behavour for writes.Now we can replace the
lock_typevariable with a flags one, where no flag means theDIO_NO_LOCKINGbehaviour andDIO_LOCKINGis kept as the first flag. Separate out the check for not allowing to fill holes into a separate flag, although for now both flags always get set at the same time.Also revamp the documentation of the locking scheme to actually make sense.
Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6/fs/ocfs2/aops.c
===================================================================fs/ocfs2/aops.c +4
-30@@ -545,6 +545,9 @@ bail:* * called like this: dio->get_blocks(dio->inode, fs_startblk, * fs_count, map_bh, dio->rw == WRITE); +* +* Note that we never bother to allocate blocks here, and thus ignore the +* create argument. */ static int ocfs2_direct_IO_get_blocks (struct inode *inode,sector_t iblock,struct buffer_head *bh_result,int create)@@ -561,14 +564,6 @@ static int ocfs2_direct_IO_get_blocks(st inode_blocks =ocfs2_blocks_for_bytes (inode->i_sb,i_size_read (inode)); -/* -* Any write past EOF is not allowed because we'd be extending. -*/ -if (create && (iblock + max_blocks) > inode_blocks) { - ret = -EIO ; - goto bail; - } -/* This figures out the size of the next contiguous block, and * our logical offset */ ret = ocfs2_extent_map_get_blocks(inode, iblock, &p_blkno,@@ -580,15 +575,6 @@ static int ocfs2_direct_IO_get_blocks(st goto bail; } -if (!ocfs2_sparse_alloc (OCFS2_SB (inode->i_sb)) && !p_blkno && create) { -ocfs2_error (inode->i_sb, -"Inode %llu has a hole at block %llu\n" , - (unsigned long long )OCFS2_I (inode)->ip_blkno, - (unsigned long long )iblock); - ret = -EROFS ; - goto bail; - } -/* * get_more_blocks() expects us to describe a hole by clearing * the mapped bit on bh_result(). @@ -597,20 +583,8 @@ static int ocfs2_direct_IO_get_blocks(st*/ if (p_blkno && !(ext_flags &OCFS2_EXT_UNWRITTEN ))map_bh (bh_result, inode->i_sb, p_blkno); -else { -/* -* ocfs2_prepare_inode_for_write() should have caught -* the case where we'd be filling a hole and triggered -* a buffered write instead. -*/ -if (create) { - ret = -EIO ; -mlog_errno (ret); - goto bail; - } - +else clear_buffer_mapped (bh_result); - }/* make sure we don't map more than max_blocks blocks here as that's all the kernel will handle at this point. */ Index:
linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===================================================================fs/xfs/linux-2.6/xfs_aops.c +7
-13@@ -1545,19 +1545,13 @@ xfs_vm_direct_IO( bdev =xfs_find_bdev_for_inode (XFS_I (inode)); -if (rw ==WRITE ) { - iocb->private =xfs_alloc_ioend (inode,IOMAP_UNWRITTEN ); - ret =blockdev_direct_IO_own_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - }else { - iocb->private =xfs_alloc_ioend (inode,IOMAP_READ ); - ret =blockdev_direct_IO_no_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - } + iocb->private =xfs_alloc_ioend (inode, rw ==WRITE ? +IOMAP_UNWRITTEN :IOMAP_READ ); + + ret =blockdev_direct_IO_no_locking (rw, iocb, inode, bdev, iov, + offset, nr_segs, + xfs_get_blocks_direct, + xfs_end_io_direct);if (unlikely (ret != -EIOCBQUEUED && iocb->private))xfs_destroy_ioend (iocb->private);Index:
linux-2.6/fs/direct-io.c
===================================================================fs/direct-io.c +4
-9@@ -53,13 +53,6 @@* * If blkfactor is zero then the user's request was aligned to the filesystem's * blocksize. -* -* lock_type is DIO_LOCKING for regular files on direct-IO-naive filesystems. -* This determines whether we need to do the fancy locking which prevents -* direct-IO from being able to read uninitialised disk blocks. If its zero -* (blockdev) this locking is not done, and if it is DIO_OWN_LOCKING i_mutex is -* not held for the entire direct write (taken briefly, initially, during a -* direct read though, but its never held for the duration of a direct-IO). */ struct dio {@@ -68,7 +61,7 @@ struct dio {struct inode *inode;int rw;loff_t i_size;/* i_size when submitted */ -int lock_type;/* doesn't change */ +int flags;/* doesn't change */ unsigned blkbits;/* doesn't change */ unsigned blkfactor;/* When we're using an alignment which is finer than the filesystem's soft @@ -240,7 +233,8 @@ static int dio_complete(struct dio *dio,if (dio->end_io && dio->result) dio->end_io (dio->iocb, offset, transferred, dio->map_bh.b_private); -if (dio->lock_type ==DIO_LOCKING ) + +if (dio->flags &DIO_LOCKING )/* lockdep: non-owner release */ up_read_non_owner (&dio->inode->i_alloc_sem);@@ -515,21 +509,24 @@ static int get_more_blocks(struct dio *d map_bh->b_state =0 ; map_bh->b_size = fs_count << dio->inode->i_blkbits; +/*
fs/direct-io.c static int get_more_blocks(struct dio *d · line 522
Quoted 10 lines
fs/direct-io.c +10
-0+* For writes inside i_size on a DIO_SKIP_HOLES filesystem we +* forbid block creations: only overwrites are permitted. +* We will return early to the caller once we see an +* unmapped buffer head returned, and the caller will fall +* back to buffered I/O. +* +* Otherwise the decision is left to the get_blocks method, +* which may decide to handle it or also return an unmapped +* buffer head. +*/
Thanks for the update.
I realized ext4 is not a pure DIO_FILL_HOLES filesystem, as it also support old ext3 format file(non-extent based), so it would need the create flag to set to 0, and the ext4 direct IO get_blocks will decide whether to fill holes or skip holes, depending on the file is extent based or not.
I feel it's better to remove the DIOS_SKIP_HOLES flag and comment that the decision is left to the get_blocks method.
Mingming
Quoted 225 lines
fs/direct-io.c +39
-69create = dio->rw &WRITE ; -if (dio->lock_type ==DIO_LOCKING ) { +if (dio->flags &DIO_SKIP_HOLES ) {if (dio->block_in_file < (i_size_read (dio->inode) >> dio->blkbits)) create =0 ; - }else if (dio->lock_type ==DIO_NO_LOCKING ) { - create =0 ; } -/* -* For writes inside i_size we forbid block creations: only -* overwrites are permitted. We fall back to buffered writes -* at a higher level for inside-i_size block-instantiating -* writes. -*/ ret = (*dio->get_block)(dio->inode, fs_startblk, map_bh, create); }@@ -1042,7 +1039,7 @@ direct_io_worker(int rw, struct kiocb *i* we can let i_mutex go now that its achieved its purpose * of protecting us from looking up uninitialized blocks. */ -if ((rw ==READ ) && (dio->lock_type ==DIO_LOCKING )) +if (rw ==READ && (dio->flags &DIO_LOCKING ))mutex_unlock (&dio->inode->i_mutex);/* @@ -1086,30 +1083,28 @@ direct_io_worker(int rw, struct kiocb *i/* * This is a library function for use by filesystem drivers. -* The locking rules are governed by the dio_lock_type parameter. * -* DIO_NO_LOCKING (no locking, for raw block device access) -* For writes, i_mutex is not held on entry; it is never taken. -* -* DIO_LOCKING (simple locking for regular files) -* For writes we are called under i_mutex and return with i_mutex held, even -* though it is internally dropped. -* For reads, i_mutex is not held on entry, but it is taken and dropped before -* returning. -* -* DIO_OWN_LOCKING (filesystem provides synchronisation and handling of -* uninitialised data, allowing parallel direct readers and writers) -* For writes we are called without i_mutex, return without it, never touch it. -* For reads we are called under i_mutex and return with i_mutex held, even -* though it may be internally dropped. -* -* Additional i_alloc_sem locking requirements described inline below. +* The locking rules are governed by the flags parameter: +* - if the flags value contains DIO_LOCKING we use a fancy locking +* scheme for dumb filesystems. +* For writes this function is called under i_mutex and returns with +* i_mutex held, for reads, i_mutex is not held on entry, but it is +* taken and dropped again before returning. +* For reads and writes i_alloc_sem is taken in shared mode and released +* on I/O completion (which may happen asynchronously after returning to +* the caller). +* +* - if the flags value does NOT contain DIO_LOCKING we don't use any +* internal locking but rather rely on the filesystem to synchronize +* direct I/O reads/writes versus each other and truncate. +* For reads and writes both i_mutex and i_alloc_sem are not held on +* entry and are never taken. */ ssize_t __blockdev_direct_IO (int rw,struct kiocb *iocb,struct inode *inode,struct block_device *bdev,const struct iovec *iov,loff_t offset,unsigned long nr_segs,get_block_t get_block,dio_iodone_t end_io, -int dio_lock_type) +int flags) {int seg;size_t size;@@ -1120,8 +1115,6 @@ __blockdev_direct_IO(int rw, struct kiocssize_t retval = -EINVAL ;loff_t end = offset;struct dio *dio; -int release_i_mutex =0 ; -int acquire_i_mutex =0 ;if (rw &WRITE ) rw =WRITE_ODIRECT ;@@ -1156,43 +1149,30 @@ __blockdev_direct_IO(int rw, struct kiocif (!dio) goto out; -/* -* For block device access DIO_NO_LOCKING is used, -* neither readers nor writers do any locking at all -* For regular files using DIO_LOCKING, -* readers need to grab i_mutex and i_alloc_sem -* writers need to grab i_alloc_sem only (i_mutex is already held) -* For regular files using DIO_OWN_LOCKING, -* neither readers nor writers take any locks here -*/ - dio->lock_type = dio_lock_type; -if (dio_lock_type !=DIO_NO_LOCKING ) { + dio->flags = flags; +if (dio->flags &DIO_LOCKING ) {/* watch out for a 0 len io from a tricksy fs */ if (rw ==READ && end > offset) { -struct address_space *mapping; +struct address_space *mapping = + iocb->ki_filp->f_mapping; - mapping = iocb->ki_filp->f_mapping; -if (dio_lock_type !=DIO_OWN_LOCKING ) { -mutex_lock (&inode->i_mutex); - release_i_mutex =1 ; - } +/* will be released by direct_io_worker */ +mutex_lock (&inode->i_mutex); retval =filemap_write_and_wait_range (mapping, offset, end -1 );if (retval) { +mutex_unlock (&inode->i_mutex);kfree (dio); goto out; } - -if (dio_lock_type ==DIO_OWN_LOCKING ) { -mutex_unlock (&inode->i_mutex); - acquire_i_mutex =1 ; - } } -if (dio_lock_type ==DIO_LOCKING ) -/* lockdep: not the owner will release it */ -down_read_non_owner (&inode->i_alloc_sem); +/* +* Will be released at I/O completion, possibly in a +* different thread. +*/ +down_read_non_owner (&inode->i_alloc_sem); }/* @@ -1210,24 +1190,19 @@ __blockdev_direct_IO(int rw, struct kioc/* * In case of error extending write may have instantiated a few * blocks outside i_size. Trim these off again for DIO_LOCKING. -* NOTE: DIO_NO_LOCK/DIO_OWN_LOCK callers have to handle this by -* it's own meaner. +* +* NOTE: filesystems with their own locking have to handle this +* on their own. */ -if (unlikely (retval <0 && (rw &WRITE ))) { -loff_t isize =i_size_read (inode); - -if (end > isize && dio_lock_type ==DIO_LOCKING ) -vmtruncate (inode, isize); +if (dio->flags &DIO_LOCKING ) { +if (unlikely ((rw &WRITE ) && retval <0 )) { +loff_t isize =i_size_read (inode); +if (end > isize ) +vmtruncate (inode, isize); + } } -if (rw ==READ && dio_lock_type ==DIO_LOCKING ) - release_i_mutex =0 ; - out: -if (release_i_mutex) -mutex_unlock (&inode->i_mutex); -else if (acquire_i_mutex) -mutex_lock (&inode->i_mutex);return retval; }EXPORT_SYMBOL (__blockdev_direct_IO);Index:
linux-2.6/include/linux/fs.h
===================================================================include/linux/fs.h +8
-15@@ -2246,9 +2246,11 @@ ssize_t __blockdev_direct_IO(int rw, str int lock_type);enum { -DIO_LOCKING =1 ,/* need locking between buffered and direct access */ -DIO_NO_LOCKING ,/* bdev; no locking at all between buffered/direct */ -DIO_OWN_LOCKING ,/* filesystem locks buffered and direct internally */ +/* need locking between buffered and direct access */ +DIO_LOCKING =0x01 , + +/* filesystem does not support filling holes */ +DIO_SKIP_HOLES =0x02 , }; static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,@@ -2257,7 +2259,8 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_LOCKING ); + nr_segs, get_block, end_io, +DIO_LOCKING |DIO_SKIP_HOLES ); } static inline ssize_t blockdev_direct_IO_no_locking(int rw, struct kiocb *iocb,@@ -2266,16 +2269,7 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_NO_LOCKING ); -} - -static inline ssize_t blockdev_direct_IO_own_locking (int rw,struct kiocb *iocb, -struct inode *inode,struct block_device *bdev,const struct iovec *iov, -loff_t offset,unsigned long nr_segs,get_block_t get_block, -dio_iodone_t end_io) -{ -return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_OWN_LOCKING ); + nr_segs, get_block, end_io,0 ); }#endif --To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
respin due to rejects in the current merge window:
--
Subject: cleanup blockdev_direct_IO locking
From: Christoph Hellwig <[email protected]>
Currently the locking in blockdev_direct_IO is a mess, we have three different locking types and very confusing checks for some of them. The most complicated one is DIO_OWN_LOCKING for reads, which happens to not actually be used.
This patch gets rid of the DIO_OWN_LOCKING - as mentioned above the read case is unused anyway, and the write side is almost identical to DIO_NO_LOCKING. The difference is that DIO_NO_LOCKING always sets the create argument for the get_blocks callback to zero, but we can easily move that to the actual get_blocks callbacks. There are four users of the DIO_NO_LOCKING mode:
gfs already ignores the create argument and thus is fine with the new version, ocfs2 only errors out if create were ever set, and we can remove this dead code now, the block device code only ever uses create for an error message if we are fully beyond the device which can never happen, and last but not least XFS will need the new behavour for writes.
Now we can replace the lock_type variable with a flags one, where no flag means the DIO_NO_LOCKING behaviour and DIO_LOCKING is kept as the first flag. Separate out the check for not allowing to fill holes into a separate flag, although for now both flags always get set at the same time.
Also revamp the documentation of the locking scheme to actually make sense.
fs/ocfs2/aops.c +4 -30
@@ -547,6 +547,9 @@ bail:* * called like this: dio->get_blocks(dio->inode, fs_startblk, * fs_count, map_bh, dio->rw == WRITE); +* +* Note that we never bother to allocate blocks here, and thus ignore the +* create argument. */ static int ocfs2_direct_IO_get_blocks (struct inode *inode,sector_t iblock,struct buffer_head *bh_result,int create)
@@ -563,14 +566,6 @@ static int ocfs2_direct_IO_get_blocks(st inode_blocks =ocfs2_blocks_for_bytes (inode->i_sb,i_size_read (inode)); -/* -* Any write past EOF is not allowed because we'd be extending. -*/ -if (create && (iblock + max_blocks) > inode_blocks) { - ret = -EIO ; - goto bail; - } -/* This figures out the size of the next contiguous block, and * our logical offset */ ret = ocfs2_extent_map_get_blocks(inode, iblock, &p_blkno,
@@ -582,15 +577,6 @@ static int ocfs2_direct_IO_get_blocks(st goto bail; } -if (!ocfs2_sparse_alloc (OCFS2_SB (inode->i_sb)) && !p_blkno && create) { -ocfs2_error (inode->i_sb, -"Inode %llu has a hole at block %llu\n" , - (unsigned long long )OCFS2_I (inode)->ip_blkno, - (unsigned long long )iblock); - ret = -EROFS ; - goto bail; - } -/* We should already CoW the refcounted extent. */ BUG_ON (ext_flags &OCFS2_EXT_REFCOUNTED );/*
@@ -601,20 +587,8 @@ static int ocfs2_direct_IO_get_blocks(st*/ if (p_blkno && !(ext_flags &OCFS2_EXT_UNWRITTEN ))map_bh (bh_result, inode->i_sb, p_blkno); -else { -/* -* ocfs2_prepare_inode_for_write() should have caught -* the case where we'd be filling a hole and triggered -* a buffered write instead. -*/ -if (create) { - ret = -EIO ; -mlog_errno (ret); - goto bail; - } - +else clear_buffer_mapped (bh_result); - }/* make sure we don't map more than max_blocks blocks here as that's all the kernel will handle at this point. */
fs/xfs/linux-2.6/xfs_aops.c +7 -13
@@ -1544,19 +1544,13 @@ xfs_vm_direct_IO( bdev =xfs_find_bdev_for_inode (XFS_I (inode)); -if (rw ==WRITE ) { - iocb->private =xfs_alloc_ioend (inode,IOMAP_UNWRITTEN ); - ret =blockdev_direct_IO_own_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - }else { - iocb->private =xfs_alloc_ioend (inode,IOMAP_READ ); - ret =blockdev_direct_IO_no_locking (rw, iocb, inode, - bdev, iov, offset, nr_segs, - xfs_get_blocks_direct, - xfs_end_io_direct); - } + iocb->private =xfs_alloc_ioend (inode, rw ==WRITE ? +IOMAP_UNWRITTEN :IOMAP_READ ); + + ret =blockdev_direct_IO_no_locking (rw, iocb, inode, bdev, iov, + offset, nr_segs, + xfs_get_blocks_direct, + xfs_end_io_direct);if (unlikely (ret != -EIOCBQUEUED && iocb->private))xfs_destroy_ioend (iocb->private);
fs/direct-io.c +53 -78
213 lines not on this page · raw message
include/linux/fs.h +8 -14
@@ -2261,9 +2261,11 @@ ssize_t __blockdev_direct_IO(int rw, str int lock_type);enum { -DIO_LOCKING =1 ,/* need locking between buffered and direct access */ -DIO_NO_LOCKING ,/* bdev; no locking at all between buffered/direct */ -DIO_OWN_LOCKING ,/* filesystem locks buffered and direct internally */ +/* need locking between buffered and direct access */ +DIO_LOCKING =0x01 , + +/* filesystem does not support filling holes */ +DIO_SKIP_HOLES =0x02 , }; static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,
@@ -2272,7 +2274,8 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_LOCKING ); + nr_segs, get_block, end_io, +DIO_LOCKING |DIO_SKIP_HOLES ); } static inline ssize_t blockdev_direct_IO_no_locking(int rw, struct kiocb *iocb,
@@ -2281,16 +2284,7 @@ static inline ssize_t blockdev_direct_IO dio_iodone_t end_io) {return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_NO_LOCKING ); -} - -static inline ssize_t blockdev_direct_IO_own_locking (int rw,struct kiocb *iocb, -struct inode *inode,struct block_device *bdev,const struct iovec *iov, -loff_t offset,unsigned long nr_segs,get_block_t get_block, -dio_iodone_t end_io) -{ -return __blockdev_direct_IO (rw, iocb, inode, bdev, iov, offset, - nr_segs, get_block, end_io,DIO_OWN_LOCKING ); + nr_segs, get_block, end_io,0 ); }#endif