Re: [PATCH 4 of 7] Turn the DIO lock_type parameter into a flags field

[David Chinner <dgc@sgi.com>]

On Wed, Nov 01, 2006 at 11:08:06AM -0400, Chris Mason wrote:
> This creates a number of flags so that filesystems can control
> blockdev_direct_IO.  It is based on code from Russell Cettelan.
> 
> The new flags are:
> DIO_CREATE -- always pass create=1 to get_block on writes.  This allows
> 	      DIO to fill holes in the file.
> DIO_PLACEHOLDERS -- use placeholder pages to provide locking against buffered
> 	            io and truncates.
> DIO_EXTEND -- use truncate to grow the file instead of falling back to
> 	      buffered io.
> DIO_DROP_I_MUTEX -- drop i_mutex before starting the IO on writes
> 
> Signed-off-by: Chris Mason <chris.mason@oracle.com>

....
> @@ -1193,28 +1190,17 @@ 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.
> - *
> - * 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 flags parameter is a bitmask of:
> + *
> + * DIO_PLACEHOLDERS (use placeholder pages for locking)
> + * DIO_CREATE (pass create=1 to get_block for filling holes)
> + * DIO_DROP_I_MUTEX (drop inode->i_mutex during writes)

DIO_EXTEND?

> @@ -1271,9 +1256,14 @@ __blockdev_direct_IO(int rw, struct kioc
>  	 * 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 && end > offset) {
> +	dio->flags = flags;
> +
> +	/*
> +	 * the placeholder code does filemap_write_and_wait, so if we
> +	 * aren't using placeholders we have to do it here
> +	 */
> +	if (!(dio->flags & DIO_PLACEHOLDERS) && end > offset) {
> +		struct address_space *mapping = iocb->ki_filp->f_mapping;
>  		retval = filemap_write_and_wait_range(mapping, offset, end - 1);
>  		if (retval)
>  			goto out;

So that means XFS will now do three cache flushes on write (one in
xfs_write(), one in generic_file_direct_IO() and yet another here....

Given the unconditional flush in generic_file_direct_IO(), is this
flush here needed at all?


> @@ -1310,7 +1301,7 @@ __blockdev_direct_IO(int rw, struct kioc
>  			if (retval)
>  				goto out;
>  		}
> -		if (is_sync_kiocb(iocb)) {
> +		if ((dio->flags & DIO_DROP_I_MUTEX) && is_sync_kiocb(iocb)) {
>  			dio->reacquire_i_mutex = 1;
>  			mutex_unlock(&inode->i_mutex);
>  		}

Hmm - do we need dio->reacquire_i_mutex? We could use the DIO_DROP_I_MUTEX
to determine if we need to reacquire the mutex (i.e. we clear the
flag if we don't need to reacquire the mutex.).

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group