Change O_DIRECT to use placeholders instead of i_mutex/i_alloc_sem locking

On Wed, Nov 01, 2006 at 11:08:04AM -0400, Chris Mason wrote:
> All mutex and semaphore usage is removed from
fs/direct-io.c.  Callers
> can ask for placeholder pages if they want help
protecting against
> races with buffered io.
> 
> Signed-off-by: Chris Mason <chris.masonoracle.com>
.....

> +static void unlock_page_range(struct dio *dio,
unsigned long start,
> +			      unsigned long nr)
> +{
> +	if (dio->lock_type != DIO_NO_LOCKING) {
>
+		remove_placeholder_pages(dio->inode->i_mapping,
dio->tmppages,
> +					 &dio->fake,
> +					 start, start + nr,
> +					 ARRAY_SIZE(dio->tmppages));
> +	}
> +}
> +
> +static int lock_page_range(struct dio *dio, unsigned
long start,
> +			   unsigned long nr)
> +{
> +	struct address_space *mapping =
dio->inode->i_mapping;
> +	struct page *fake = &dio->fake;
> +	unsigned long end = start + nr;
> +
> +	if (dio->lock_type == DIO_NO_LOCKING)
> +		return 0;
> +	return find_or_insert_placeholders(mapping,
dio->tmppages, start, end,
> +	                                 
ARRAY_SIZE(dio->tmppages),
> +					  GFP_KERNEL, fake,
> +					  dio->rw == READ);
> +}

I'm not sure that these two function are well named -
lock_page_range() could be confused for an extenstion of
lock_page()
and they are very different things. Something like
dio_lock_range()
and dio_unlock_range() might be more appropriate...

> +
> +
>  /*
>   * Get another userspace page.  Returns an ERR_PTR on
error.  Pages are
>   * buffered inside the dio so that we can call
get_user_pages() against a
>  -219,9 +255,9  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)
> -		/* lockdep: non-owner release */
>
-		up_read_non_owner(&dio->inode->i_alloc_sem);
> +	unlock_page_range(dio, dio->fspages_start_off,
> +			  dio->fspages_end_off -
dio->fspages_start_off);
> +	dio->fspages_end_off = dio->fspages_start_off;
>  }

dio->fspages_end_off and dio->fspages_start_off only
seem to have
relevance to the DIO_PLACEHOLDER case - can this be wrapped
up
inside dio_unlock_range()?

>  
>  /*
>  -517,6 +553,7  static int get_more_blocks(struct dio *d
>  	unsigned long fs_count;	/* Number of filesystem-sized
blocks */
>  	unsigned long dio_count;/* Number of dio_block-sized
blocks */
>  	unsigned long blkmask;
> +	unsigned long index;
>  	int create;
>  
>  	/*
>  -544,7 +581,19  static int get_more_blocks(struct dio *d
>  		} else if (dio->lock_type == DIO_NO_LOCKING) {
>  			create = 0;
>  		}
> -
> +	        index = fs_startblk >>
(PAGE_CACHE_SHIFT -
> +		                       
dio->inode->i_blkbits);
> +		end = (dio->final_block_in_request >>
dio->blkfactor) >>
> +		      (PAGE_CACHE_SHIFT -
dio->inode->i_blkbits);
> +		BUG_ON(index > end);
> +		while (index >= dio->fspages_end_off) {
> +			unsigned long nr;
> +			nr = min(end - index + 1, (unsigned
long)DIO_PAGES);
> +			ret = lock_page_range(dio, dio->fspages_end_off,
nr);
> +			if (ret)
> +				goto error;
> +			dio->fspages_end_off += nr;
> +		}

Ditto for this - if we are not using placeholders, then none
of this
is needed and so could be wrapped in:

		if (dio->flags & DIO_PLACEHOLDER) {
			ret = dio_lock_range(dio, ...)
			if (ret)
				goto error;
		}

>  -1221,49 +1257,29  __blockdev_direct_IO(int
rw, struct kioc
>  				goto out;
>  		}
>  	}
> -
>  	dio = kmalloc(sizeof(*dio), GFP_KERNEL);
>  	retval = -ENOMEM;
>  	if (!dio)
>  		goto out;
>  
> +	set_page_placeholder(&dio->fake);
> +	dio->fspages_start_off = offset >>
PAGE_CACHE_SHIFT;
> +	dio->fspages_end_off = dio->fspages_start_off;
> +

if (flags & DIO_PLACEHOLDER) ?

>  	/*
>  	 * 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)
> +	 *	No locks are taken
>  	 * For regular files using DIO_OWN_LOCKING,
>  	 *	neither readers nor writers take any locks here
>  	 */

So nothing does any locking here - comment can be
simplified/killed?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group
-
To unsubscribe from this list: send the line
"unsubscribe linux-fsdevel" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html