Re: [PATCH] add check do_direct_IO() return val

Joe Jin wrote:

I think we still want to run dio_cleanup() if do_direct_IO() failed? Otherwise we can leak pages.

And there's nothing special about EFAULT or ENOMEM here: if do_direct_IO()
returns any error then that's it: we bale out, yes?

Yes, I think we'll out from here if get EFAULT/ENOMEM error, also maybe -EIO
return, return diretly should ok.

In fact I'm suspecting that this is what the code in there used to do. Something like:

	for (...) {
		...
		ret = do_direct_IO(...);
		...
		if (ret) {
			dio_dleanup(dio);
			break
		}
	}
	return ret;

Yes, we need call dio_cleanup() to release page cache, I lost it.

However, we need do more while return -ENOTBLK, right?
so I think the patch maybe like following:


--- linux-2.6.22/fs/direct-io.c.orig	2007-07-27 14:39:15.000000000 +0800
+++ linux-2.6.22/fs/direct-io.c	2007-07-27 15:08:58.000000000 +0800
@@ -1032,18 +1032,19 @@ direct_io_worker(int rw, struct kiocb *i
 					blkbits);

if (ret) {

+			if (ret == -ENOTBLK && (rw & WRITE)) {
+				/*
+				 * The remaining part of the request will be
+				 * be handled by buffered I/O when we return
+				 */
+				ret = 0;
+				break;
+			}
 			dio_cleanup(dio);
-			break;
+			goto out;
 		}
 	} /* end iovec loop */

- if (ret == -ENOTBLK && (rw & WRITE)) {

-		/*
-		 * The remaining part of the request will be
-		 * be handled by buffered I/O when we return
-		 */
-		ret = 0;
-	}
 	/*
 	 * There may be some unwritten disk at the end of a part-written
 	 * fs-block-sized block.  Go zero that now.

_

However I'd like to ask you guys to carefully review and test that please.

Gurudas, would you please give more test of this patch?

I tested Andrew's patch and panic was gone but got few ENOTBLK.
So I tried with Joe's patch , both panic and ENOTBLK are gone now.
But in Joe's patch if (ret == -ENOTBLK && (rw & WRITE)), dio_cleanup(dio)

was not getting called because of break. So I moved dio_cleanup just after if (ret).

I tested with this patch, both panic and ENOTBLK are gone now.


Here is the modified patch:

--- linux-2.6.22/fs/direct-io.c.orig 2007-07-27 03:06:39.000000000 -0700
+++ linux-2.6.22/fs/direct-io.c 2007-07-27 03:24:55.000000000 -0700
@@ -1033,17 +1033,18 @@

if (ret) {
dio_cleanup(dio);
- break;
+ if (ret == -ENOTBLK && (rw & WRITE)) {
+ /*
+ * The remaining part of the request will be
+ * be handled by buffered I/O when we return
+ */
+ ret = 0;
+ break;
+ }
+ goto out;
}
} /* end iovec loop */

- if (ret == -ENOTBLK && (rw & WRITE)) {
- /*
- * The remaining part of the request will be
- * be handled by buffered I/O when we return
- */
- ret = 0;
- }
/*
* There may be some unwritten disk at the end of a part-written
* fs-block-sized block. Go zero that now.
@@ -1112,7 +1113,7 @@
kfree(dio);
} else
BUG_ON(ret != -EIOCBQUEUED);
-
+out:
return ret;
}


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

---

• Follow-Ups:
  • Re: [PATCH] add check do_direct_IO() return val
    • From: Joe Jin <[email protected]>

• References:
  • [PATCH] add check do_direct_IO() return val
    • From: Joe Jin <[email protected]>
  • Re: [PATCH] add check do_direct_IO() return val
    • From: Andrew Morton <[email protected]>
  • Re: [PATCH] add check do_direct_IO() return val
    • From: Joe Jin <[email protected]>

• Prev by Date: Re: [PATCH/RFC] remove frv usage of flush_tlb_pgtables()
• Next by Date: [PATCH] make Oprofile call shutdown() only once per session
• Previous by thread: Re: [PATCH] add check do_direct_IO() return val
• Next by thread: Re: [PATCH] add check do_direct_IO() return val
• Index(es):
  • Date
  • Thread

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]