On Fri, 2006-10-13 at 12:48 +1000, David Chinner wrote: > On Thu, Oct 12, 2006 at 07:56:38PM -0500, Russell Cattelan wrote: > > While trying to fix up GFS2 directio and reading through the code > > involving the various lock flags I discovered the DIO_OWN_LOCKING > > flag is no longer used. > > > > XFS recently changed it xfs_vm_direct_IO function to call > > blockdev_direct_IO_no_locking for reads and > > blockdev_direct_IO_own_locking > > for writes. But DIO_OWN_LOCKING is only used in the direct IO read case > > so effectively the flag is never checked an therefore can probably be > > removed. > > NACK. > > This breaks XFS direct writes - the DIO_OWN_LOCKING flag has meaning > for direct writes even though a simple grep doesn't give you any > hits. get_more_blocks() sets the create flag unconditionally on > writes when DIO_OWN_LOCKING is set, and this is needed for XFS to be > able to allocate underlying blocks if the direct write is over a > hole or past EOF. Arrghh you are correct! Even more reason to clean this logic up. look this version over and see what you think. comments not in final state but is describing what is being changed an why. Basically the idea is to have separate flags for locking and creation, overloading the flags meant that they were specific for XFS needs and therefore did not work for GFS. Also go to a TRUE state if flag on and a FALSE state if flag off. vs the mix of true flag (DIO_LOCKING) vs false flag (DIO_NO_LOCKING) The other option might be to have xfs pass different get_blocks functions for read and write, but that seems even more confusing based on the presence of create flag. And probably wouldn't help GFS in the long run unless it did the same get_block_read/get_block_write thing. -- Russell Cattelan <cattelan@xxxxxxxxxxx>
Index: work_gfs/fs/direct-io.c
===================================================================
--- work_gfs.orig/fs/direct-io.c 2006-10-13 12:32:57.641898963 -0500
+++ work_gfs/fs/direct-io.c 2006-10-13 12:35:46.032216555 -0500
@@ -56,9 +56,7 @@
* 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).
+ * (blockdev) this locking is not done.
*/
struct dio {
@@ -67,7 +65,7 @@ struct dio {
struct inode *inode;
int rw;
loff_t i_size; /* i_size when submitted */
- int lock_type; /* doesn't change */
+ unsigned 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
@@ -219,7 +217,7 @@ static void dio_complete(struct dio *dio
{
if (dio->end_io && dio->result)
dio->end_io(dio->iocb, offset, bytes, 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);
}
@@ -537,12 +535,15 @@ static int get_more_blocks(struct dio *d
map_bh->b_size = fs_count << dio->inode->i_blkbits;
create = dio->rw & WRITE;
- if (dio->lock_type == DIO_LOCKING) {
- if (dio->block_in_file < (i_size_read(dio->inode) >>
- dio->blkbits))
+ if (!(dio->flags & DIO_CREATE)) {
+ if (dio->flags & DIO_LOCKING) {
+ if (dio->block_in_file < (i_size_read(dio->inode) >>
+ dio->blkbits))
+ create = 0;
+ } else {
+ /* locks not held, allocate off */
create = 0;
- } else if (dio->lock_type == DIO_NO_LOCKING) {
- create = 0;
+ }
}
/*
@@ -1080,7 +1081,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);
/*
@@ -1157,9 +1158,11 @@ 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.
+ * The locking rules are governed by the dio_flags parameter.
+ * (previously dio_lock_type)
*
- * DIO_NO_LOCKING (no locking, for raw block device access)
+ * DIO_NO_LOCKING is now the LACK of the DIO_LOCKING flag.
+ * (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)
@@ -1168,19 +1171,13 @@ direct_io_worker(int rw, struct kiocb *i
* 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.
*/
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)
+ unsigned int dio_flags)
{
int seg;
size_t size;
@@ -1191,8 +1188,6 @@ __blockdev_direct_IO(int rw, struct kioc
ssize_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_SYNC;
@@ -1233,37 +1228,26 @@ __blockdev_direct_IO(int rw, struct kioc
* 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 = dio_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;
-
mapping = iocb->ki_filp->f_mapping;
- if (dio_lock_type != DIO_OWN_LOCKING) {
- mutex_lock(&inode->i_mutex);
- release_i_mutex = 1;
- }
+
+ 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);
+ } /* else write */
+ down_read_non_owner(&inode->i_alloc_sem);
}
/*
@@ -1278,14 +1262,7 @@ __blockdev_direct_IO(int rw, struct kioc
retval = direct_io_worker(rw, iocb, inode, iov, offset,
nr_segs, blkbits, get_block, end_io, dio);
- 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: work_gfs/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- work_gfs.orig/fs/xfs/linux-2.6/xfs_aops.c 2006-10-13 12:31:44.748938391 -0500
+++ work_gfs/fs/xfs/linux-2.6/xfs_aops.c 2006-10-13 12:35:46.160218368 -0500
@@ -1389,19 +1389,18 @@ xfs_vm_direct_IO(
iocb->private = xfs_alloc_ioend(inode, IOMAP_UNWRITTEN);
- if (rw == WRITE) {
- ret = blockdev_direct_IO_own_locking(rw, iocb, inode,
- iomap.iomap_target->bt_bdev,
- iov, offset, nr_segs,
- xfs_get_blocks_direct,
- xfs_end_io_direct);
- } else {
- ret = blockdev_direct_IO_no_locking(rw, iocb, inode,
- iomap.iomap_target->bt_bdev,
- iov, offset, nr_segs,
- xfs_get_blocks_direct,
- xfs_end_io_direct);
- }
+ /* direct IO is called without the DIO_LOCKING flag,
+ * which imply the previous behavior of the DIO_NO_LOCKING
+ * flag. DIO_CREATE now says always pass create = 1 when
+ * dio->rw == WRITE
+ */
+
+ ret = blockdev_direct_IO_flags(rw, iocb, inode,
+ iomap.iomap_target->bt_bdev,
+ iov, offset, nr_segs,
+ xfs_get_blocks_direct,
+ xfs_end_io_direct,
+ DIO_CREATE);
if (unlikely(ret <= 0 && iocb->private))
xfs_destroy_ioend(iocb->private);
Index: work_gfs/include/linux/fs.h
===================================================================
--- work_gfs.orig/include/linux/fs.h 2006-10-13 12:31:45.516948375 -0500
+++ work_gfs/include/linux/fs.h 2006-10-13 12:35:46.224219274 -0500
@@ -1718,39 +1718,39 @@ static inline void do_generic_file_read(
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 lock_type);
+ unsigned int dio_flags);
-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 */
-};
+#define DIO_LOCKING (1 << 0) /* need locking between buffered and direct access */
+#define DIO_CREATE (1 << 1) /* file system can always create new blocks */
static inline 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)
{
+ /* locking is on, create is condtional */
return __blockdev_direct_IO(rw, iocb, inode, bdev, iov, offset,
nr_segs, get_block, end_io, DIO_LOCKING);
}
-static inline ssize_t blockdev_direct_IO_no_locking(int rw, struct kiocb *iocb,
+static inline ssize_t blockdev_direct_IO_flags(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)
+ dio_iodone_t end_io, unsigned int flags)
{
+ /* file system dictates locking and create behavior */
return __blockdev_direct_IO(rw, iocb, inode, bdev, iov, offset,
- nr_segs, get_block, end_io, DIO_NO_LOCKING);
+ nr_segs, get_block, end_io, flags);
}
-static inline ssize_t blockdev_direct_IO_own_locking(int rw, struct kiocb *iocb,
+static inline ssize_t blockdev_direct_IO_no_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)
{
+ /* locking is off, create is off */
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);
}
extern const struct file_operations generic_ro_fops;
Attachment:
signature.asc
Description: This is a digitally signed message part
