Re: [patch rfc] towards supporting O_NONBLOCK on regular files

[Marcelo Tosatti <marcelo.tosatti@cyclades.com>]

On Fri, Oct 01, 2004 at 04:57:50PM -0400, Jeff Moyer wrote:
> This patch makes an attempt at supporting the O_NONBLOCK flag for regular
> files.  It's pretty straight-forward.  One limitation is that we still call
> into the readahead code, which I believe can block.  However, if we don't
> do this, then an application which only uses non-blocking reads may never
> get it's data.
> 
> Comments welcome.

Hi Jeff,

Curiosity: Is this defined in any UNIX standard?

Adv. Programming in the UNIX environment says:

12.2 Nonblocking I/O

"Nonblocking I/O lets us issue an I/O operation, such as open, read,
or write and not have it block forever. If the operation cannot be 
completed, return is made immediately with an error noting that 
the operation would have blocked."

He is talking about read's on descriptors (pipe's, devices, etc) which 
block in case of no data present, not about filesystem IO.

But here we create our own semantics of O_NONBLOCK on read() of 
fs IO. As you say page_cache_readahead can block for one
trying to allocate pages, possibly while submitting IO too.

The patch is cool - might be nice to check if SuS or someone else
specificies behaviour and try to match if so?

> 
> -Jeff
> 
> --- linux-2.6.8/mm/filemap.c.orig	2004-09-30 16:33:46.881129560 -0400
> +++ linux-2.6.8/mm/filemap.c	2004-09-30 16:34:12.109294296 -0400
> @@ -720,7 +720,7 @@ void do_generic_mapping_read(struct addr
>  	unsigned long index, end_index, offset;
>  	loff_t isize;
>  	struct page *cached_page;
> -	int error;
> +	int error, nonblock = filp->f_flags & O_NONBLOCK;
>  	struct file_ra_state ra = *_ra;
>  
>  	cached_page = NULL;
> @@ -755,10 +755,20 @@ find_page:
>  		page = find_get_page(mapping, index);
>  		if (unlikely(page == NULL)) {
>  			handle_ra_miss(mapping, &ra, index);
> +			if (nonblock) {
> +				desc->error = -EWOULDBLOCK;
> +				break;
> +			}
>  			goto no_cached_page;
>  		}
> -		if (!PageUptodate(page))
> +		if (!PageUptodate(page)) {
> +			if (nonblock) {
> +				page_cache_release(page);
> +				desc->error = -EWOULDBLOCK;
> +				break;
> +			}
>  			goto page_not_up_to_date;
> +		}
>  page_ok:
>  
>  		/* If users can be writing to this page using arbitrary
> @@ -1004,7 +1014,7 @@ __generic_file_aio_read(struct kiocb *io
>  			desc.error = 0;
>  			do_generic_file_read(filp,ppos,&desc,file_read_actor);
>  			retval += desc.written;
> -			if (!retval) {
> +			if (!retval || desc.error) {
>  				retval = desc.error;
>  				break;
>  			}