[PATCH] add check do_direct_IO() return val

[PATCH] add check do_direct_IO() return val

[Joe Jin]

This is the patch for check do_direct_IO() return val.

At do_direct_IO(), sometimes dio_get_page() will return -EFAULT/-ENOMEM,
according to orig source, it will go on left work. buf for dio_get_page()
return a error will made many useful member of dio not initialized like
dio->map_bh and others, at this point, kernel will panic.

Signed-off-by: Joe Jin <joe.jin@oracle.com>


---
--- linux-2.6.22/fs/direct-io.c.orig	2007-07-26 11:32:27.000000000 +0800
+++ linux-2.6.22/fs/direct-io.c	2007-07-26 11:33:58.000000000 +0800
@@ -1031,7 +1031,9 @@ direct_io_worker(int rw, struct kiocb *i
 			((dio->final_block_in_request - dio->block_in_file) <<
 					blkbits);
 
-		if (ret) {
+		if (ret == -EFAULT || ret == -ENOMEM) 
+			goto out;
+		else if (ret) {
 			dio_cleanup(dio);
 			break;
 		}
@@ -1113,6 +1115,7 @@ direct_io_worker(int rw, struct kiocb *i
 	} else
 		BUG_ON(ret != -EIOCBQUEUED);
 
+out:
 	return ret;
 }
 

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

[Andrew Morton]

On Thu, 26 Jul 2007 17:04:00 +0800 Joe Jin <joe.jin@oracle.com> wrote:

> This is the patch for check do_direct_IO() return val.
> 
> At do_direct_IO(), sometimes dio_get_page() will return -EFAULT/-ENOMEM,
> according to orig source, it will go on left work. buf for dio_get_page()
> return a error will made many useful member of dio not initialized like
> dio->map_bh and others, at this point, kernel will panic.
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> 
> 
> ---
> --- linux-2.6.22/fs/direct-io.c.orig	2007-07-26 11:32:27.000000000 +0800
> +++ linux-2.6.22/fs/direct-io.c	2007-07-26 11:33:58.000000000 +0800
> @@ -1031,7 +1031,9 @@ direct_io_worker(int rw, struct kiocb *i
>  			((dio->final_block_in_request - dio->block_in_file) <<
>  					blkbits);
>  
> -		if (ret) {
> +		if (ret == -EFAULT || ret == -ENOMEM) 
> +			goto out;
> +		else if (ret) {
>  			dio_cleanup(dio);
>  			break;
>  		}
> @@ -1113,6 +1115,7 @@ direct_io_worker(int rw, struct kiocb *i
>  	} else
>  		BUG_ON(ret != -EIOCBQUEUED);
>  
> +out:
>  	return ret;
>  }
>  

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?

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;

but then someone later came along and added more code between the end of
the for loop and the `return' statement.

<I do miss the BK diffviewer sometimes>

<does a binary search>

yep, that's exactly what happened.  We broke it in the 2.5.45 release back
in October 2002. (Where "we" == "Badari")

So I think what we need to do is something close to this:

--- a/fs/direct-io.c~add-check-do_direct_io-return-val
+++ a/fs/direct-io.c
@@ -1033,7 +1033,7 @@ direct_io_worker(int rw, struct kiocb *i
 
 		if (ret) {
 			dio_cleanup(dio);
-			break;
+			goto out;
 		}
 	} /* end iovec loop */
 
@@ -1112,7 +1112,7 @@ direct_io_worker(int rw, struct kiocb *i
 		kfree(dio);
 	} else
 		BUG_ON(ret != -EIOCBQUEUED);
-
+out:
 	return ret;
 }
 
_

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

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

[wengang wang]

Hi all,
Add some backgrounds:

When doing fio test on kernel 2.6.22,  we got oops,
--------------------------------------------------------------
BUG: unable to handle kernel paging request at virtual address 23c070bf
 printing eip:
c04a07fd
*pdpt = 000000001ff88001
*pde = 0000000000000000
Oops: 0000 [#1]
SMP
Modules linked in: netconsole autofs4 hidp nfs lockd nfs_acl rfcomm l2cap
bluetooth sunrpc ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr
/@ iscsi_tcp libiscsi scsi_transport_iscsi dm_mirror dm_multipath dm_mod 
video /
sbs button battery ac ipv6 parport_pc lp parport i2c_piix4 i2c_core 
cfi_probe
gen_probe floppy scb2_flash sg mtdcore chipreg tg3 e1000 serio_raw ide_cd
/@ cdrom aic7xxx scsi_transport_spi sd_mod scsi_mod ext3 jbd ehci_hcd 
ohci_hcd /
uhci_hcd
CPU:    0
EIP:    0060:[<c04a07fd>]    Not tainted VLI
EFLAGS: 00010293   (2.6.22 #2)
EIP is at bio_get_nr_vecs+0x0/0x30
eax: 23c07063   ebx: 00000003   ecx: ffffffff   edx: 00000000
esi: de5cef74   edi: f54a9600   ebp: 00000000   esp: de5ceca8
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process fio (pid: 17820, ti=de5ce000 task=de6570e0 task.ti=de5ce000)
Stack: c04a1c9d ffffffff ffffffff 00000009 f54a9600 de5cef74 00000000
f54a9600
       c04a1f43 00000000 c04a2b46 c0460466 c2c5baa0 c0812500 c0462c0a
00000001
       00000001 df4b90d4 de5ceee4 00000011 00000001 00000009 00000009
00000000
Call Trace:
 [<c04a1c9d>] dio_new_bio+0x82/0xfe
 [<c04a1f43>] dio_send_cur_page+0x4a/0x92
 [<c04a2b46>] __blockdev_direct_IO+0xa09/0xc83
 [<c0460466>] __pagevec_free+0x14/0x1a
 [<c0462c0a>] release_pages+0x137/0x13f
 [<f8856f30>] journal_start+0xaf/0xdd [jbd]
 [<f8890fec>] ext3_direct_IO+0xfd/0x190 [ext3]
 [<f888f6af>] ext3_get_block+0x0/0xd0 [ext3]
 [<c045d803>] generic_file_direct_IO+0xe5/0x116
 [<c045d890>] generic_file_direct_write+0x5c/0x137
 [<c045e285>] __generic_file_aio_write_nolock+0x37b/0x4df
 [<c045e43e>] generic_file_aio_write+0x55/0xb3
 [<f888cfdc>] ext3_file_write+0x24/0x8f [ext3]
 [<c0481af9>] do_sync_write+0xc7/0x10a
 [<c04347d2>] check_kill_permission+0xec/0xf5
 [<c043c557>] autoremove_wake_function+0x0/0x35
 [<c0481a32>] do_sync_write+0x0/0x10a
 [<c048233e>] vfs_write+0xa8/0x154
/@  [<c0482a1a>] sys_pwrite64+0x48/0x5f/
 [<c0404e12>] syscall_call+0x7/0xb
 [<c0620000>] xfrm_replay_timer_handler+0x3e/0x44
 =======================
Code: 89 c5 c7 44 24 14 f4 ff ff ff 74 d2 e9 b3 fe ff ff 83 7c 24 34 00 
0f 84
0b ff ff ff e9 51 ff ff ff 83 c4 20 89 e8 5b 5e 5f 5d c3 <8b> 40 5c 8b 
48 38
8b 81 20 01 00 00 0f b7 91 2a 01 00 00 0f b7
EIP: [<c04a07fd>] bio_get_nr_vecs+0x0/0x30 SS:ESP 0068:de5ceca8

-----------------------------------------------------------

jobfile is
-------------------------------
/@ [global]/
/@ bs=8k/
/@ iodepth=1024/
/@ iodepth_batch=60/
/@ randrepeat=1/
/@ size=1m/
/@ directory=/home/oracle/
/@ numjobs=20/
/@ [job1]/
/@ ioengine=sync/
/@ bs=1k/
/@ direct=1/
/@ rw=randread/
/@ filename=file1:file2/
/@ [job2]/
/@ ioengine=libaio/
/@ rw=randwrite/
/@ direct=1/
/@ filename=file1:file2/
/@ [job3]/
/@ bs=1k/
/@ ioengine=posixaio/
/@ rw=randwrite/
/@ direct=1/
/@ filename=file1:file2/
/@ [job4]/
/@ ioengine=splice/
/@ direct=1/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job5]/
/@ bs=1k/
/@ ioengine=sync/
/@ rw=randread/
/@ filename=file1:file2/
/@ [job7]/
/@ ioengine=libaio/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job8]/
/@ ioengine=posixaio/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job9]/
/@ ioengine=splice/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job10]/
/@ ioengine=mmap/
/@ rw=randwrite/
/@ bs=1k/
/@ filename=file1:file2/
/@ [job11]/
/@ ioengine=mmap/
/@ rw=randwrite/
/@ direct=1/
/@ filename=file1:file2/
-------------------------------
ignore the @ please.


With Joe's patch, seems the oops solved.
So, please give a review to see if there is any problem for that patch.

thanks,
wengang.

**Joe Jin wrote:
> This is the patch for check do_direct_IO() return val.
>
> At do_direct_IO(), sometimes dio_get_page() will return -EFAULT/-ENOMEM,
> according to orig source, it will go on left work. buf for dio_get_page()
> return a error will made many useful member of dio not initialized like
> dio->map_bh and others, at this point, kernel will panic.
>
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>
>
> ---
> --- linux-2.6.22/fs/direct-io.c.orig	2007-07-26 11:32:27.000000000 +0800
> +++ linux-2.6.22/fs/direct-io.c	2007-07-26 11:33:58.000000000 +0800
> @@ -1031,7 +1031,9 @@ direct_io_worker(int rw, struct kiocb *i
>  			((dio->final_block_in_request - dio->block_in_file) <<
>  					blkbits);
>  
> -		if (ret) {
> +		if (ret == -EFAULT || ret == -ENOMEM) 
> +			goto out;
> +		else if (ret) {
>  			dio_cleanup(dio);
>  			break;
>  		}
> @@ -1113,6 +1115,7 @@ direct_io_worker(int rw, struct kiocb *i
>  	} else
>  		BUG_ON(ret != -EIOCBQUEUED);
>  
> +out:
>  	return ret;
>  }
>  
>   

-- 
Wengang Wang
Member of Technical Staff
Oracle Asia R&D Center
Open Source Technologies Development

Tel:      +86 10 8278 6265
Mobile:   +86 13381078925

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

[Joe Jin]

> 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?

Thanks, 
Joe

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

[Dave Young]

>On 7/27/07, Joe Jin <joe.jin@oracle.com> 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?
>
Hi,
If you are sure the problem is caused by EFAULT or ENOMEM , why not:
               if (ret) {
                       dio_cleanup(dio);
                       if (ret == -EFAULT || ret == -ENOMEM)
                               goto out;
                       break;
               }

regards
dave

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

[Joe Jin]

> If you are sure the problem is caused by EFAULT or ENOMEM , why not:

To this issue(wengang have posted the background), I hit it for 
do_direct_IO() return EFAULT caused the panic, however, the function
not only return these critical errors, maybe other error return like
EIO.

Thanks,
Joe

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

[gurudas pai]

Andrew Morton wrote:
> On Thu, 26 Jul 2007 17:04:00 +0800 Joe Jin <joe.jin@oracle.com> wrote:
>
>> This is the patch for check do_direct_IO() return val.
>>
>> At do_direct_IO(), sometimes dio_get_page() will return -EFAULT/-ENOMEM,
>> according to orig source, it will go on left work. buf for dio_get_page()
>> return a error will made many useful member of dio not initialized like
>> dio->map_bh and others, at this point, kernel will panic.
>>
>> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>>
>>
>> ---
>> --- linux-2.6.22/fs/direct-io.c.orig	2007-07-26 11:32:27.000000000 +0800
>> +++ linux-2.6.22/fs/direct-io.c	2007-07-26 11:33:58.000000000 +0800
>> @@ -1031,7 +1031,9 @@ direct_io_worker(int rw, struct kiocb *i
>>  			((dio->final_block_in_request - dio->block_in_file) <<
>>  					blkbits);
>>  
>> -		if (ret) {
>> +		if (ret == -EFAULT || ret == -ENOMEM) 
>> +			goto out;
>> +		else if (ret) {
>>  			dio_cleanup(dio);
>>  			break;
>>  		}
>> @@ -1113,6 +1115,7 @@ direct_io_worker(int rw, struct kiocb *i
>>  	} else
>>  		BUG_ON(ret != -EIOCBQUEUED);
>>  
>> +out:
>>  	return ret;
>>  }
>>  
>
> 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?
>
> 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;
>
> but then someone later came along and added more code between the end of
> the for loop and the `return' statement.
>
> <I do miss the BK diffviewer sometimes>
>
> <does a binary search>
>
> yep, that's exactly what happened.  We broke it in the 2.5.45 release back
> in October 2002. (Where "we" == "Badari")
>
> So I think what we need to do is something close to this:
>
> --- a/fs/direct-io.c~add-check-do_direct_io-return-val
> +++ a/fs/direct-io.c
> @@ -1033,7 +1033,7 @@ direct_io_worker(int rw, struct kiocb *i
>  
>  		if (ret) {
>  			dio_cleanup(dio);
> -			break;
> +			goto out;
>  		}
>  	} /* end iovec loop */
>  
> @@ -1112,7 +1112,7 @@ direct_io_worker(int rw, struct kiocb *i
>  		kfree(dio);
>  	} else
>  		BUG_ON(ret != -EIOCBQUEUED);
> -
> +out:
>  	return ret;
>  }
>  
> _
>
> However I'd like to ask you guys to carefully review and test that please.
Tested Andrew's patch , works! . My test case passed without any panic. 
But got couple of ENOTBLK.


Thanks,
-Guru

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

[gurudas pai]

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;
}

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

[Joe Jin]

> 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).

Guru, actually, break from the loop with ENOTBLK will call dio_cleanup
at leater, if call it too early, that means will put_page(), maybe cause
other panic.

Thanks,
Joe

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

[Andrew Morton]

On Sat, 28 Jul 2007 11:47:19 +0800
Joe Jin <joe.jin@oracle.com> wrote:

> > 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).
> 
> Guru, actually, break from the loop with ENOTBLK will call dio_cleanup
> at leater, if call it too early, that means will put_page(), maybe cause
> other panic.
> 

fyi, I dropped the earlier patch and now we have nothing.  Please let's get all
this sorted out in time for 2.6.23.  Which is still many weeks away so there is
plenty of time to prepare something which was carefully reviewed and well-tested,
thanks.

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

[Zach Brown]

>
> fyi, I dropped the earlier patch and now we have nothing.  Please  
> let's get all
> this sorted out in time for 2.6.23.  Which is still many weeks away  
> so there is
> plenty of time to prepare something which was carefully reviewed  
> and well-tested,
> thanks.

I was distracted by OSCON last week.  I'm working on reproducing this  
and wrapping my head around it as we speak.  We'll get it sorted out.

- z

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

[Badari Pulavarty]

On Mon, 2007-07-30 at 13:53 -0700, Andrew Morton wrote:
> On Sat, 28 Jul 2007 11:47:19 +0800
> Joe Jin <joe.jin@oracle.com> wrote:
> 
> > > 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).
> > 
> > Guru, actually, break from the loop with ENOTBLK will call dio_cleanup
> > at leater, if call it too early, that means will put_page(), maybe cause
> > other panic.
> > 
> 
> fyi, I dropped the earlier patch and now we have nothing.  Please let's get all
> this sorted out in time for 2.6.23.  Which is still many weeks away so there is
> plenty of time to prepare something which was carefully reviewed and well-tested,
> thanks.

I am also taking a look at it right now. Unfortunately, I don't think
fix is that simple - since we need to return success, in case of a
partial write.

Thanks,
Badari

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

[Zach Brown]

> I am also taking a look at it right now.

Are we having a race to write a little test app that reproduces the  
problem? :)

- z

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

[Badari Pulavarty]

On Mon, 2007-07-30 at 14:45 -0700, Zach Brown wrote:
> > I am also taking a look at it right now.
> 
> Are we having a race to write a little test app that reproduces the  
> problem? :)

Nope. Feel free to write the test case. I am just looking at the code
to see what needs to be done.

Thanks,
Badari

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

[Zach Brown]

On Jul 30, 2007, at 2:58 PM, Badari Pulavarty wrote:

> On Mon, 2007-07-30 at 14:45 -0700, Zach Brown wrote:
>>> I am also taking a look at it right now.
>>
>> Are we having a race to write a little test app that reproduces the
>> problem? :)
>
> Nope. Feel free to write the test case. I am just looking at the code
> to see what needs to be done.

OK, sounds good.

- z

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

[Zach Brown]

On Jul 30, 2007, at 2:58 PM, Badari Pulavarty wrote:

> On Mon, 2007-07-30 at 14:45 -0700, Zach Brown wrote:
>>> I am also taking a look at it right now.
>>
>> Are we having a race to write a little test app that reproduces the
>> problem? :)
>
> Nope. Feel free to write the test case.

Well, I'm having a heck of a time getting this to fail.  It looks  
possible, though.  Joe, were you guys able to narrow it down to a  
reproducible test case?  Do you have any oops output messages from  
the crashes?

It looks like it takes a very particular set of circumstances to  
actually crash after relying on an uninitialized map_bh.  (see the  
blkfactor, buffer_new(), and this_chunk_blocks tests in dio_zero_block 
()).

> I am just looking at the code
> to see what needs to be done.

It looks like the unconditional dio_cleanup() and dio_zero_block()  
calls outside the nseg loop are relying on state which might not have  
been built up.  _zero_block() tests map_bh's flags without them being  
set.  _cleanup could, in some crazy world, get confused if we managed  
to get here with a 0 nr_segs because dio->head and ->tail wouldn't be  
initialized.

So we could initialize some more fields at the start of  
direct_io_worker for the benefit of these cleanup calls.  Or we could  
conditionally call them based on some other indicator of progress.   
Neither really thrills me.

And I don't have a test case to verify changes with.  Meh.

How do you feel about initializing the dio with kzalloc() and only  
initializing the fields that we rely on being non-zero, and  
commenting the hell out of it?

- z

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

[Badari Pulavarty]

On Mon, 2007-07-30 at 16:38 -0700, Zach Brown wrote:
> On Jul 30, 2007, at 2:58 PM, Badari Pulavarty wrote:
> 
> > On Mon, 2007-07-30 at 14:45 -0700, Zach Brown wrote:
> >>> I am also taking a look at it right now.
> >>
> >> Are we having a race to write a little test app that reproduces the
> >> problem? :)
> >
> > Nope. Feel free to write the test case.
> 
> Well, I'm having a heck of a time getting this to fail.  It looks  
> possible, though.  Joe, were you guys able to narrow it down to a  
> reproducible test case?  Do you have any oops output messages from  
> the crashes?
> 
> It looks like it takes a very particular set of circumstances to  
> actually crash after relying on an uninitialized map_bh.  (see the  
> blkfactor, buffer_new(), and this_chunk_blocks tests in dio_zero_block 
> ()).


Looking at the crash

CPU:    0
EIP:    0060:[<c04a07fd>]    Not tainted VLI
EFLAGS: 00010293   (2.6.22 #2)
EIP is at bio_get_nr_vecs+0x0/0x30
eax: 23c07063   ebx: 00000003   ecx: ffffffff   edx: 00000000
esi: de5cef74   edi: f54a9600   ebp: 00000000   esp: de5ceca8
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process fio (pid: 17820, ti=de5ce000 task=de6570e0 task.ti=de5ce000)
Stack: c04a1c9d ffffffff ffffffff 00000009 f54a9600 de5cef74 00000000
f54a9600
       c04a1f43 00000000 c04a2b46 c0460466 c2c5baa0 c0812500 c0462c0a
00000001
       00000001 df4b90d4 de5ceee4 00000011 00000001 00000009 00000009
00000000
Call Trace:
 [<c04a1c9d>] dio_new_bio+0x82/0xfe
 [<c04a1f43>] dio_send_cur_page+0x4a/0x92
 [<c04a2b46>] __blockdev_direct_IO+0xa09/0xc83
 [<c0460466>] __pagevec_free+0x14/0x1a
 [<c0462c0a>] release_pages+0x137/0x13f
 [<f8856f30>] journal_start+0xaf/0xdd [jbd]
 [<f8890fec>] ext3_direct_IO+0xfd/0x190 [ext3]
..

I am not sure, I really understand whats happening here. It looks
like dio->map_bh.b_dev is junk. But looking at the code, we shouldn't
be in dio_send_cur_page() unless if we have a valid dio->cur_page.
(which means, that we added a page to submit, which also means
previous getblock() succeded, which means that we should have a
valid map_bh). What am I missing here ?


        if (dio->cur_page) {
                ret2 = dio_send_cur_page(dio);
                if (ret == 0)
                        ret = ret2;
                page_cache_release(dio->cur_page);
                dio->cur_page = NULL;
        }

> 
> > I am just looking at the code
> > to see what needs to be done.
> 
> It looks like the unconditional dio_cleanup() and dio_zero_block()  
> calls outside the nseg loop are relying on state which might not have  
> been built up.  _zero_block() tests map_bh's flags without them being  
> set.  _cleanup could, in some crazy world, get confused if we managed  
> to get here with a 0 nr_segs because dio->head and ->tail wouldn't be  
> initialized.
> 
> So we could initialize some more fields at the start of  
> direct_io_worker for the benefit of these cleanup calls.  Or we could  
> conditionally call them based on some other indicator of progress.   
> Neither really thrills me.
> 
> And I don't have a test case to verify changes with.  Meh.
> 
> How do you feel about initializing the dio with kzalloc() and only  
> initializing the fields that we rely on being non-zero, and  
> commenting the hell out of it?

Yeah. kzalloc() may be right way to go.. But I would like to understand
what exactly is happening here.

Thanks,
Badari

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

[Badari Pulavarty]

On Mon, 2007-07-30 at 16:38 -0700, Zach Brown wrote:
> On Jul 30, 2007, at 2:58 PM, Badari Pulavarty wrote:
> 
> > On Mon, 2007-07-30 at 14:45 -0700, Zach Brown wrote:
> >>> I am also taking a look at it right now.
> >>
> >> Are we having a race to write a little test app that reproduces the
> >> problem? :)
> >
> > Nope. Feel free to write the test case.
> 
> Well, I'm having a heck of a time getting this to fail.  It looks  
> possible, though.  Joe, were you guys able to narrow it down to a  
> reproducible test case?  Do you have any oops output messages from  
> the crashes?

Here is what I got earlier..

Thanks,
Badari


Hi all,
Add some backgrounds:

When doing fio test on kernel 2.6.22,  we got oops,
--------------------------------------------------------------
BUG: unable to handle kernel paging request at virtual address 23c070bf
 printing eip:
c04a07fd
*pdpt = 000000001ff88001
*pde = 0000000000000000
Oops: 0000 [#1]
SMP
Modules linked in: netconsole autofs4 hidp nfs lockd nfs_acl rfcomm l2cap
bluetooth sunrpc ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr
/@ iscsi_tcp libiscsi scsi_transport_iscsi dm_mirror dm_multipath dm_mod 
video /
sbs button battery ac ipv6 parport_pc lp parport i2c_piix4 i2c_core 
cfi_probe
gen_probe floppy scb2_flash sg mtdcore chipreg tg3 e1000 serio_raw ide_cd
/@ cdrom aic7xxx scsi_transport_spi sd_mod scsi_mod ext3 jbd ehci_hcd 
ohci_hcd /
uhci_hcd
CPU:    0
EIP:    0060:[<c04a07fd>]    Not tainted VLI
EFLAGS: 00010293   (2.6.22 #2)
EIP is at bio_get_nr_vecs+0x0/0x30
eax: 23c07063   ebx: 00000003   ecx: ffffffff   edx: 00000000
esi: de5cef74   edi: f54a9600   ebp: 00000000   esp: de5ceca8
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process fio (pid: 17820, ti=de5ce000 task=de6570e0 task.ti=de5ce000)
Stack: c04a1c9d ffffffff ffffffff 00000009 f54a9600 de5cef74 00000000
f54a9600
       c04a1f43 00000000 c04a2b46 c0460466 c2c5baa0 c0812500 c0462c0a
00000001
       00000001 df4b90d4 de5ceee4 00000011 00000001 00000009 00000009
00000000
Call Trace:
 [<c04a1c9d>] dio_new_bio+0x82/0xfe
 [<c04a1f43>] dio_send_cur_page+0x4a/0x92
 [<c04a2b46>] __blockdev_direct_IO+0xa09/0xc83
 [<c0460466>] __pagevec_free+0x14/0x1a
 [<c0462c0a>] release_pages+0x137/0x13f
 [<f8856f30>] journal_start+0xaf/0xdd [jbd]
 [<f8890fec>] ext3_direct_IO+0xfd/0x190 [ext3]
 [<f888f6af>] ext3_get_block+0x0/0xd0 [ext3]
 [<c045d803>] generic_file_direct_IO+0xe5/0x116
 [<c045d890>] generic_file_direct_write+0x5c/0x137
 [<c045e285>] __generic_file_aio_write_nolock+0x37b/0x4df
 [<c045e43e>] generic_file_aio_write+0x55/0xb3
 [<f888cfdc>] ext3_file_write+0x24/0x8f [ext3]
 [<c0481af9>] do_sync_write+0xc7/0x10a
 [<c04347d2>] check_kill_permission+0xec/0xf5
 [<c043c557>] autoremove_wake_function+0x0/0x35
 [<c0481a32>] do_sync_write+0x0/0x10a
 [<c048233e>] vfs_write+0xa8/0x154
/@  [<c0482a1a>] sys_pwrite64+0x48/0x5f/
 [<c0404e12>] syscall_call+0x7/0xb
 [<c0620000>] xfrm_replay_timer_handler+0x3e/0x44
 =======================
Code: 89 c5 c7 44 24 14 f4 ff ff ff 74 d2 e9 b3 fe ff ff 83 7c 24 34 00 
0f 84
0b ff ff ff e9 51 ff ff ff 83 c4 20 89 e8 5b 5e 5f 5d c3 <8b> 40 5c 8b 
48 38
8b 81 20 01 00 00 0f b7 91 2a 01 00 00 0f b7
EIP: [<c04a07fd>] bio_get_nr_vecs+0x0/0x30 SS:ESP 0068:de5ceca8

-----------------------------------------------------------

jobfile is
-------------------------------
/@ [global]/
/@ bs=8k/
/@ iodepth=1024/
/@ iodepth_batch=60/
/@ randrepeat=1/
/@ size=1m/
/@ directory=/home/oracle/
/@ numjobs=20/
/@ [job1]/
/@ ioengine=sync/
/@ bs=1k/
/@ direct=1/
/@ rw=randread/
/@ filename=file1:file2/
/@ [job2]/
/@ ioengine=libaio/
/@ rw=randwrite/
/@ direct=1/
/@ filename=file1:file2/
/@ [job3]/
/@ bs=1k/
/@ ioengine=posixaio/
/@ rw=randwrite/
/@ direct=1/
/@ filename=file1:file2/
/@ [job4]/
/@ ioengine=splice/
/@ direct=1/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job5]/
/@ bs=1k/
/@ ioengine=sync/
/@ rw=randread/
/@ filename=file1:file2/
/@ [job7]/
/@ ioengine=libaio/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job8]/
/@ ioengine=posixaio/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job9]/
/@ ioengine=splice/
/@ rw=randwrite/
/@ filename=file1:file2/
/@ [job10]/
/@ ioengine=mmap/
/@ rw=randwrite/
/@ bs=1k/
/@ filename=file1:file2/
/@ [job11]/
/@ ioengine=mmap/
/@ rw=randwrite/
/@ direct=1/
/@ filename=file1:file2/
-------------------------------
ignore the @ please.


With Joe's patch, seems the oops solved.
So, please give a review to see if there is any problem for that patch.

thanks,
wengang.

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

[Joe Jin]

> Well, I'm having a heck of a time getting this to fail.  It looks  
> possible, though.  Joe, were you guys able to narrow it down to a  
> reproducible test case?  Do you have any oops output messages from  
> the crashes?

Zach, it easy to reproduce through fio with following config file

# cat jobfile
[global]
bs=8k
iodepth=1024
iodepth_batch=60
randrepeat=1
size=1m
directory=/home/oracle
numjobs=20
[job1]
	ioengine=sync
	bs=1k
	direct=1
	rw=randread
	filename=file1:file2
[job2]
	ioengine=libaio
	rw=randwrite
	direct=1
	filename=file1:file2
[job3]
	bs=1k
	ioengine=posixaio
	rw=randwrite
	direct=1
	filename=file1:file2
[job4]
	ioengine=splice
	direct=1
	rw=randwrite
	filename=file1:file2
[job5]
	bs=1k
	ioengine=sync
	rw=randread
	filename=file1:file2
[job7]
	ioengine=libaio
	rw=randwrite
	filename=file1:file2
[job8]
	ioengine=posixaio
	rw=randwrite
	filename=file1:file2
[job9]
	ioengine=splice
	rw=randwrite
	filename=file1:file2
[job10]
	ioengine=mmap
	rw=randwrite
	bs=1k
	filename=file1:file2
[job11]
	ioengine=mmap
	rw=randwrite
	direct=1
	filename=file1:file2 



Runing fio just a short time, will get panic like following:


BUG: unable to handle kernel paging request at virtual address 23c070bf
 printing eip:
c04a07fd
*pdpt = 000000001ff88001
*pde = 0000000000000000
Oops: 0000 [#1]
SMP
Modules linked in: netconsole autofs4 hidp nfs lockd nfs_acl rfcomm l2cap
bluetooth sunrpc ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr
@ iscsi_tcp libiscsi scsi_transport_iscsi dm_mirror dm_multipath dm_mod video
sbs button battery ac ipv6 parport_pc lp parport i2c_piix4 i2c_core cfi_probe
gen_probe floppy scb2_flash sg mtdcore chipreg tg3 e1000 serio_raw ide_cd
@ cdrom aic7xxx scsi_transport_spi sd_mod scsi_mod ext3 jbd ehci_hcd ohci_hcd
uhci_hcd
CPU:    0
EIP:    0060:[<c04a07fd>]    Not tainted VLI
EFLAGS: 00010293   (2.6.22 #2)
EIP is at bio_get_nr_vecs+0x0/0x30
eax: 23c07063   ebx: 00000003   ecx: ffffffff   edx: 00000000
esi: de5cef74   edi: f54a9600   ebp: 00000000   esp: de5ceca8
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process fio (pid: 17820, ti=de5ce000 task=de6570e0 task.ti=de5ce000)
Stack: c04a1c9d ffffffff ffffffff 00000009 f54a9600 de5cef74 00000000 f54a9600
       c04a1f43 00000000 c04a2b46 c0460466 c2c5baa0 c0812500 c0462c0a 00000001
       00000001 df4b90d4 de5ceee4 00000011 00000001 00000009 00000009 00000000
Call Trace:
 [<c04a1c9d>] dio_new_bio+0x82/0xfe
 [<c04a1f43>] dio_send_cur_page+0x4a/0x92
 [<c04a2b46>] __blockdev_direct_IO+0xa09/0xc83
 [<c0460466>] __pagevec_free+0x14/0x1a
 [<c0462c0a>] release_pages+0x137/0x13f
 [<f8856f30>] journal_start+0xaf/0xdd [jbd]
 [<f8890fec>] ext3_direct_IO+0xfd/0x190 [ext3]
 [<f888f6af>] ext3_get_block+0x0/0xd0 [ext3]
 [<c045d803>] generic_file_direct_IO+0xe5/0x116
 [<c045d890>] generic_file_direct_write+0x5c/0x137
 [<c045e285>] __generic_file_aio_write_nolock+0x37b/0x4df
 [<c045e43e>] generic_file_aio_write+0x55/0xb3
 [<f888cfdc>] ext3_file_write+0x24/0x8f [ext3]
 [<c0481af9>] do_sync_write+0xc7/0x10a
 [<c04347d2>] check_kill_permission+0xec/0xf5
 [<c043c557>] autoremove_wake_function+0x0/0x35
 [<c0481a32>] do_sync_write+0x0/0x10a
 [<c048233e>] vfs_write+0xa8/0x154
 [<c0482a1a>] sys_pwrite64+0x48/0x5f
 [<c0404e12>] syscall_call+0x7/0xb
 [<c0620000>] xfrm_replay_timer_handler+0x3e/0x44
 =======================
Code: 89 c5 c7 44 24 14 f4 ff ff ff 74 d2 e9 b3 fe ff ff 83 7c 24 34 00 0f 84
0b ff ff ff e9 51 ff ff ff 83 c4 20 89 e8 5b 5e 5f 5d c3 <8b> 40 5c 8b 48 38
8b 81 20 01 00 00 0f b7 91 2a 01 00 00 0f b7
EIP: [<c04a07fd>] bio_get_nr_vecs+0x0/0x30 SS:ESP 0068:de5ceca8 




I have tried to trace the code, panic reason is dio->map_bh.b_bdev not init by 
direct_io_worker(). at do_direct_IO, dio_get_page() will return EFAULT at
this issue, it caused the dio->map_bh cannot init by later code and direct
return a error. but at direct_io_worker() do not handle for the error.

Thanks,
Joe

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

[Badari]

Joe Jin wrote:
>> Well, I'm having a heck of a time getting this to fail.  It looks  
>> possible, though.  Joe, were you guys able to narrow it down to a  
>> reproducible test case?  Do you have any oops output messages from  
>> the crashes?
>>     
>
> Zach, it easy to reproduce through fio with following config file
>
> # cat jobfile
> [global]
> bs=8k
> iodepth=1024
> iodepth_batch=60
> randrepeat=1
> size=1m
> directory=/home/oracle
> numjobs=20
> [job1]
> 	ioengine=sync
> 	bs=1k
> 	direct=1
> 	rw=randread
> 	filename=file1:file2
> [job2]
> 	ioengine=libaio
> 	rw=randwrite
> 	direct=1
> 	filename=file1:file2
> [job3]
> 	bs=1k
> 	ioengine=posixaio
> 	rw=randwrite
> 	direct=1
> 	filename=file1:file2
> [job4]
> 	ioengine=splice
> 	direct=1
> 	rw=randwrite
> 	filename=file1:file2
> [job5]
> 	bs=1k
> 	ioengine=sync
> 	rw=randread
> 	filename=file1:file2
> [job7]
> 	ioengine=libaio
> 	rw=randwrite
> 	filename=file1:file2
> [job8]
> 	ioengine=posixaio
> 	rw=randwrite
> 	filename=file1:file2
> [job9]
> 	ioengine=splice
> 	rw=randwrite
> 	filename=file1:file2
> [job10]
> 	ioengine=mmap
> 	rw=randwrite
> 	bs=1k
> 	filename=file1:file2
> [job11]
> 	ioengine=mmap
> 	rw=randwrite
> 	direct=1
> 	filename=file1:file2 
>
>   
Hmm.. in this config file, whats causing DIO to panic ? Which test actually
passing faulty buffer ?

Thanks,
Badari

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

[Joe Jin]

> Hmm.. in this config file, whats causing DIO to panic ? Which test actually
> passing faulty buffer ?
> 

By my testing, just defined job3 and job10 will also get the panic, but if
only have one of them, panic will not appear. the faulty buffer maybe passed
by mmap.

Thanks,
Joe

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

[Badari]

Joe Jin wrote:
>> Hmm.. in this config file, whats causing DIO to panic ? Which test actually
>> passing faulty buffer ?
>>
>>     
>
> By my testing, just defined job3 and job10 will also get the panic, but if
> only have one of them, panic will not appear. the faulty buffer maybe passed
> by mmap.
>   

Thanks. looks lile my machine crashed too while running the tests. I 
will take a look.

Thanks,
Badari

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

[Badari Pulavarty]

On Tue, 2007-07-31 at 12:35 +0800, Joe Jin wrote:
> > Hmm.. in this config file, whats causing DIO to panic ? Which test actually
> > passing faulty buffer ?
> > 
> 
> By my testing, just defined job3 and job10 will also get the panic, but if
> only have one of them, panic will not appear. the faulty buffer maybe passed
> by mmap.

Okay. Here is the fix for the problem.

Here is whats happening in this case:

Faulty user-buffer caused -EFAULT to be returned from do_direct_IO().
We go into dio_zero_block() to see if we need to zero out sections of
the block (if IO size < blocksize case). It checks if the buffer is
newly allocation by doing buffer_new(bh). map_bh is NOT initialized and
never went through get_block() code. So, its possible to pass the check
and end up submitting a page wrongly and causing the oops.

Fix is to initialize the buffer state. 

Thanks,
Badari

Need to initialize map_bh.b_state to zero. Otherwise, in case of
a faulty user-buffer its possible to go into dio_zero_block()
and submit a page by mistake - since it checks for buffer_new().

http://marc.info/?l=linux-kernel&m=118551339032528&w=2

Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com>
---
 fs/direct-io.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.23-rc1/fs/direct-io.c
===================================================================
--- linux-2.6.23-rc1.orig/fs/direct-io.c	2007-07-22 13:41:00.000000000 -0700
+++ linux-2.6.23-rc1/fs/direct-io.c	2007-07-31 15:13:44.000000000 -0700
@@ -974,6 +974,7 @@ direct_io_worker(int rw, struct kiocb *i
 	dio->get_block = get_block;
 	dio->end_io = end_io;
 	dio->map_bh.b_private = NULL;
+	dio->map_bh.b_state = 0;
 	dio->final_block_in_bio = -1;
 	dio->next_block_for_io = -1;
 

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

[Andrew Morton]

On Tue, 31 Jul 2007 15:25:11 -0700
Badari Pulavarty <pbadari@us.ibm.com> wrote:

> +	dio->map_bh.b_state = 0;

ho hum, thanks.

We zero out so many fields in there now that a kzalloc() might yield
a net gain.  0.000001% in an unnamed benchmark!

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

[Linus Torvalds]

On Tue, 31 Jul 2007, Andrew Morton wrote:
> 
> We zero out so many fields in there now that a kzalloc() might yield
> a net gain.  0.000001% in an unnamed benchmark!

Indeed. That's *especially* true since right now it passes a mostly 
totally uninitialized structure pointer down to direct_io_worker(), but it 
has actually initialized a _few_ fields. Ie the "die" pointer is almost 
totally filled with random crap, except for two members: dio->lock_type 
and dio->is_async have been initialized the rest is random crud.

That kind of mixing of totally-but-not-entirely-uninitialized pointer 
passing is just a recipe for disaster.

Does a patch like this work? I don't have any test-cases, but it would be 
good to have something like this tested and passed back with proper 
explanations and sign-offs.

		Linus

---
 fs/direct-io.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 52bb263..901dc55 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -958,35 +958,22 @@ direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode,
 	ssize_t ret2;
 	size_t bytes;
 
-	dio->bio = NULL;
 	dio->inode = inode;
 	dio->rw = rw;
 	dio->blkbits = blkbits;
 	dio->blkfactor = inode->i_blkbits - blkbits;
-	dio->start_zero_done = 0;
-	dio->size = 0;
 	dio->block_in_file = offset >> blkbits;
-	dio->blocks_available = 0;
-	dio->cur_page = NULL;
 
-	dio->boundary = 0;
-	dio->reap_counter = 0;
 	dio->get_block = get_block;
 	dio->end_io = end_io;
-	dio->map_bh.b_private = NULL;
 	dio->final_block_in_bio = -1;
 	dio->next_block_for_io = -1;
 
-	dio->page_errors = 0;
-	dio->io_error = 0;
-	dio->result = 0;
 	dio->iocb = iocb;
 	dio->i_size = i_size_read(inode);
 
 	spin_lock_init(&dio->bio_lock);
 	dio->refcount = 1;
-	dio->bio_list = NULL;
-	dio->waiter = NULL;
 
 	/*
 	 * In case of non-aligned buffers, we may need 2 more
@@ -994,8 +981,6 @@ direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode,
 	 */
 	if (unlikely(dio->blkfactor))
 		dio->pages_in_io = 2;
-	else
-		dio->pages_in_io = 0;
 
 	for (seg = 0; seg < nr_segs; seg++) {
 		user_addr = (unsigned long)iov[seg].iov_base;
@@ -1183,7 +1168,7 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
 		}
 	}
 
-	dio = kmalloc(sizeof(*dio), GFP_KERNEL);
+	dio = kzalloc(sizeof(*dio), GFP_KERNEL);
 	retval = -ENOMEM;
 	if (!dio)
 		goto out;

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

[Badari Pulavarty]

On Tue, 2007-07-31 at 15:34 -0700, Andrew Morton wrote:
> On Tue, 31 Jul 2007 15:25:11 -0700
> Badari Pulavarty <pbadari@us.ibm.com> wrote:
> 
> > +	dio->map_bh.b_state = 0;
> 
> ho hum, thanks.
> 
> We zero out so many fields in there now that a kzalloc() might yield
> a net gain.  0.000001% in an unnamed benchmark!

Yep. I think its worth doing a kzalloc(). I wanted to understand the
*actual*  problem to make sure we are addressing it, rather than 
kzalloc() made it disappear.

Thanks,
Badari

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

[Zach Brown]

> Okay. Here is the fix for the problem.

Yeah, that looks right from my understanding too.  Thanks, Badari.

> Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com>

  Acked-by: Zach Brown <zach.brown@oracle.com>

- z

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

[Zach Brown]

> Does a patch like this work? I don't have any test-cases, but it  
> would be
> good to have something like this tested and passed back with proper
> explanations and sign-offs.

It should, yeah.

I'm bringing up a more capable IO testbed over here.  I'll run it  
through the tests I have once that setup.  I wouldn't cry if someone  
beat me to it ;).

- z

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

[Joe Jin]

> Okay. Here is the fix for the problem.

It work fine, thanks.

> Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com>
  
  Acked-by:  Joe Jin <joe.jin@oracle.com>

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

[Joe Jin]

> Does a patch like this work? I don't have any test-cases, but it would be 
> good to have something like this tested and passed back with proper 
> explanations and sign-offs.

Yes it work find after apply the patch, thanks.

Joe

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

[gurudas pai]

Joe Jin wrote:
>> Does a patch like this work? I don't have any test-cases, but it would be 
>> good to have something like this tested and passed back with proper 
>> explanations and sign-offs.
>
> Yes it work find after apply the patch, thanks.
>
> Joe
I tried with Badari's and Linus's patch using same fio jobfile which 
earlier produced this oops.
And it works for me.

Thanks,
-Guru