Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Andi Kleen
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A 0x) | (B 0x)) The problem is pretty nasty unfortunately. To solve it properly I think the file_operations-read/write prototypes would need to be

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Bodo Eggert
Trond Myklebust [EMAIL PROTECTED] wrote: On Mon, 2008-01-28 at 05:38 +0100, Andi Kleen wrote: On Monday 28 January 2008 05:13:09 Trond Myklebust wrote: On Mon, 2008-01-28 at 03:58 +0100, Andi Kleen wrote: The problem is that it's not a race in who gets to do its thing first, but a

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Andi Kleen
On Monday 28 January 2008 13:56:05 Alan Cox wrote: No specific spec, just general quality of implementation. I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A 0x) | (B 0x)) Agree entirely:

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Alan Cox
No specific spec, just general quality of implementation. I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A 0x) | (B 0x)) Agree entirely: the spec doesn't allow for random scribbling in the wrong

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Alan Cox
On Mon, 28 Jan 2008 15:10:34 +0100 Andi Kleen [EMAIL PROTECTED] wrote: On Monday 28 January 2008 14:38:57 Alan Cox wrote: Also worse really fixing it would be a major change to the VFS because of the way -read/write are defined :/ I don't see a problem there. -read and -write update

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Diego Calleja
El Mon, 28 Jan 2008 15:10:34 +0100, Andi Kleen [EMAIL PROTECTED] escribió: So you get overlapping reads. Probably not good. This was discussed in the past i think - http://lkml.org/lkml/2006/4/13/124 http://lkml.org/lkml/2006/4/13/130 - To unsubscribe from this list: send the line unsubscribe

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Steve French
On Jan 28, 2008 2:17 AM, Andi Kleen [EMAIL PROTECTED] wrote: I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A 0x) | (B 0x)) The problem is pretty nasty unfortunately. To solve it properly I think

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Dave Kleikamp
On Mon, 2008-01-28 at 12:33 -0600, Steve French wrote: On Jan 28, 2008 2:17 AM, Andi Kleen [EMAIL PROTECTED] wrote: I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A 0x) | (B 0x)) The

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Andi Kleen
On Monday 28 January 2008 14:38:57 Alan Cox wrote: Also worse really fixing it would be a major change to the VFS because of the way -read/write are defined :/ I don't see a problem there. -read and -write update the passed pointer which is not the real f_pos anyway. Just the copies need

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-28 Thread Alan Cox
Also worse really fixing it would be a major change to the VFS because of the way -read/write are defined :/ I don't see a problem there. -read and -write update the passed pointer which is not the real f_pos anyway. Just the copies need fixing. Alan - To unsubscribe from this list: send the

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-27 Thread Steve French
Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from f_pos in fs/read_write.c in remote_llseek with your patch since the reads/writes from that field are not necessarily atomic and threads

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-27 Thread Trond Myklebust
On Sun, 2008-01-27 at 10:57 -0600, Steve French wrote: Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from f_pos in fs/read_write.c in remote_llseek with your patch since the

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-27 Thread Steve French
If two seeks overlap, can't you end up with an f_pos value that is different than what either thread seeked to? or if you have a seek and a read overlap can't you end up with the read occurring in the midst of an update of f_pos (which takes more than one instruction on various architectures),

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-27 Thread Trond Myklebust
On Sun, 2008-01-27 at 16:18 -0600, Steve French wrote: If two seeks overlap, can't you end up with an f_pos value that is different than what either thread seeked to? or if you have a seek and a read overlap can't you end up with the read occurring in the midst of an update of f_pos (which

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-27 Thread Andi Kleen
On Sunday 27 January 2008 17:57:14 Steve French wrote: Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from f_pos in fs/read_write.c in remote_llseek with your patch since the

Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek

2008-01-27 Thread Trond Myklebust
On Mon, 2008-01-28 at 03:58 +0100, Andi Kleen wrote: The problem is that it's not a race in who gets to do its thing first, but a parallel reader can actually see a corrupted value from the two independent words on 32bit (e.g. during a 4GB). And this could actually completely corrupt