Re: [PATCH 2/4] direct-io: make O_DIRECT IO path be page based

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 20 Aug 2009 12:17:38 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> Currently we pass in the iovec array and let the O_DIRECT core
> handle the get_user_pages() business. This work, but it means that
> we can ever only use user pages for O_DIRECT.
> 
> Switch the aops->direct_IO() and below code to use page arrays
> instead, so that it doesn't make any assumptions about who the pages
> belong to. This works directly for all users but NFS, which just
> uses the same helper that the generic mapping read/write functions
> also call.
>

This:

> +	struct page **pages;		/* page buffer */
> +	unsigned int head_page;		/* next page to process */
> +	unsigned int total_pages;	/* last valid page + 1 */
> +	unsigned int first_page_off;	/* offset into first page in map */

Is a disaster waiting to happen.

>  static struct page *dio_get_page(struct dio *dio)
>  {
> -	if (dio_pages_present(dio) == 0) {
> -		int ret;
> +	if (dio->head_page < dio->total_pages)
> +		return dio->pages[dio->head_page++];
>  
> -		ret = dio_refill_pages(dio);
> -		if (ret)
> -			return ERR_PTR(ret);
> -		BUG_ON(dio_pages_present(dio) == 0);
> -	}
> -	return dio->pages[dio->head++];
> +	return NULL;
>  }
>
> ...
>
> @@ -351,8 +280,10 @@ static void dio_bio_submit(struct dio *dio)
>   */
>  static void dio_cleanup(struct dio *dio)
>  {
> -	while (dio_pages_present(dio))
> -		page_cache_release(dio_get_page(dio));
> +	struct page *page;
> +
> +	while ((page = dio_get_page(dio)) != NULL)
> +		page_cache_release(page);
>  }

What is the value of dio->head_page here?

How are callers to use this function at the start of the operation, at
the end and partway through is IO error, EFAULT etc is detected?

There's good potential for double-releases, leaks etc here.  We need to
be very careful and explicit in documenting the usage and state and
meaning of head_page, and the protocol around managing it.

It's just like that ghastly bio.bi_idx thing - how many bugs did we add
and have to fix due to that thing?

Really I think it'd be better to just nuke .head_page altogether and get
callers to manage their index openly.