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

[Jeff Moyer]

==> Regarding Re: [patch rfc] towards supporting O_NONBLOCK on regular files; "Stephen C. Tweedie" <sct@xxxxxxxxxx> adds:

sct> Hi, On Mon, 2004-10-11 at 19:58, Jeff Moyer wrote:

sct> I think it's worth getting this right in the long term, though.
sct> Getting readahead of indirect blocks right has other benefits too ---
sct> eg. we may be able to fix the situation where we end up trying to read
sct> indirect blocks before we've even submitted the IO for the previous
sct> data blocks, breaking the IO pipeline ordering.
>> So for the short term, are you an advocate of the patch posted?

sct> In the short term, can't we just disable readahead for O_NONBLOCK?
sct> That has true non-blocking semantics --- if the data is already
sct> available we return it, but if not, it's up to somebody else to
sct> retrieve it.

sct> That's exactly what you want if you're genuinely trying to avoid
sct> blocking at all costs on a really hot event loop, and the semantics
sct> seem to make sense to me.  It's not that different from the networking
sct> case where no amount of read() on a non-blocking fd will get you more
sct> data unless there's another process somewhere filling the stream.

Yes, that sounds like a fine idea.  Here is a patch which does this.
Andrew, I know you only want bug fixes, but I'd like to get this into your
queue for post 2.6.9, if possible.

Thanks,

Jeff

--- linux-2.6.9-rc4-mm1/mm/filemap.c~	2004-10-12 12:40:01.000000000 -0400
+++ linux-2.6.9-rc4-mm1/mm/filemap.c	2004-10-12 12:53:13.202396656 -0400
@@ -692,7 +692,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;
@@ -721,16 +721,27 @@ void do_generic_mapping_read(struct addr
 		nr = nr - offset;
 
 		cond_resched();
-		page_cache_readahead(mapping, &ra, filp, index);
+		if (!nonblock)
+			page_cache_readahead(mapping, &ra, filp, index);
 
 find_page:
 		page = find_get_page(mapping, index);
 		if (unlikely(page == NULL)) {
+			if (nonblock) {
+				desc->error = -EWOULDBLOCK;
+				break;
+			}
 			handle_ra_miss(mapping, &ra, index);
 			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
@@ -976,7 +987,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;
 			}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/