* [PATCH, RFC] xip: use i_mutex for xip_file_fault
@ 2011-09-10 15:31 Marco Stornelli
2011-09-10 15:56 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Marco Stornelli @ 2011-09-10 15:31 UTC (permalink / raw)
To: Linux FS Devel; +Cc: Linux Kernel
From: Marco Stornelli <marco.stornelli@gmail.com>
There aren't sufficient sync points for a fs for xip operations. In
particular for the mmap case. It can be not sufficient to lock/unlock
to do some operation inside get_xip_mem callback. For xip_file_read
it's really easy to write a fs specific wrapper, xip_file_write hold
i_mutex so no problem. With this patch we can avoid concurrent truncate
operation and xip mmap.
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
---
--- filemap_xip.c.orig 2011-09-10 17:16:56.000000000 +0200
+++ filemap_xip.c 2011-09-10 17:19:25.000000000 +0200
@@ -230,6 +230,7 @@ static int xip_file_fault(struct vm_area
int error;
/* XXX: are VM_FAULT_ codes OK? */
+ mutex_lock(&inode->i_mutex);
again:
size = (i_size_read(inode) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
if (vmf->pgoff >= size)
@@ -239,8 +240,10 @@ again:
&xip_mem, &xip_pfn);
if (likely(!error))
goto found;
- if (error != -ENODATA)
+ if (error != -ENODATA) {
+ mutex_unlock(&inode->i_mutex);
return VM_FAULT_OOM;
+ }
/* sparse block */
if ((vma->vm_flags & (VM_WRITE | VM_MAYWRITE)) &&
@@ -253,17 +256,22 @@ again:
error = mapping->a_ops->get_xip_mem(mapping, vmf->pgoff, 1,
&xip_mem, &xip_pfn);
mutex_unlock(&xip_sparse_mutex);
- if (error)
+ if (error) {
+ mutex_unlock(&inode->i_mutex);
return VM_FAULT_SIGBUS;
+ }
/* unmap sparse mappings at pgoff from all other vmas */
__xip_unmap(mapping, vmf->pgoff);
found:
err = vm_insert_mixed(vma, (unsigned long)vmf->virtual_address,
xip_pfn);
- if (err == -ENOMEM)
+ if (err == -ENOMEM) {
+ mutex_unlock(&inode->i_mutex);
return VM_FAULT_OOM;
+ }
BUG_ON(err);
+ mutex_unlock(&inode->i_mutex);
return VM_FAULT_NOPAGE;
} else {
int err, ret = VM_FAULT_OOM;
@@ -292,6 +300,7 @@ found:
out:
write_seqcount_end(&xip_sparse_seq);
mutex_unlock(&xip_sparse_mutex);
+ mutex_unlock(&inode->i_mutex);
return ret;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault
2011-09-10 15:31 [PATCH, RFC] xip: use i_mutex for xip_file_fault Marco Stornelli
@ 2011-09-10 15:56 ` Al Viro
2011-09-11 8:25 ` Marco Stornelli
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2011-09-10 15:56 UTC (permalink / raw)
To: Marco Stornelli; +Cc: Linux FS Devel, Linux Kernel
On Sat, Sep 10, 2011 at 05:31:39PM +0200, Marco Stornelli wrote:
> From: Marco Stornelli <marco.stornelli@gmail.com>
>
> There aren't sufficient sync points for a fs for xip operations. In
> particular for the mmap case. It can be not sufficient to lock/unlock
> to do some operation inside get_xip_mem callback. For xip_file_read
> it's really easy to write a fs specific wrapper, xip_file_write hold
> i_mutex so no problem. With this patch we can avoid concurrent truncate
> operation and xip mmap.
Umm... I really don't like that; what's going to happen if you have a file
mmapped and do write() to that file from address in that mapping?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault
2011-09-10 15:56 ` Al Viro
@ 2011-09-11 8:25 ` Marco Stornelli
2011-09-11 11:15 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Marco Stornelli @ 2011-09-11 8:25 UTC (permalink / raw)
To: Al Viro; +Cc: Linux FS Devel, Linux Kernel
Il 10/09/2011 17:56, Al Viro ha scritto:
> On Sat, Sep 10, 2011 at 05:31:39PM +0200, Marco Stornelli wrote:
>> From: Marco Stornelli<marco.stornelli@gmail.com>
>>
>> There aren't sufficient sync points for a fs for xip operations. In
>> particular for the mmap case. It can be not sufficient to lock/unlock
>> to do some operation inside get_xip_mem callback. For xip_file_read
>> it's really easy to write a fs specific wrapper, xip_file_write hold
>> i_mutex so no problem. With this patch we can avoid concurrent truncate
>> operation and xip mmap.
>
> Umm... I really don't like that; what's going to happen if you have a file
> mmapped and do write() to that file from address in that mapping?
>
Nothing strange. There is a serialization of the operations. Maybe I
don't understand the point here.
Marco
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault
2011-09-11 8:25 ` Marco Stornelli
@ 2011-09-11 11:15 ` Al Viro
2011-09-11 11:25 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2011-09-11 11:15 UTC (permalink / raw)
To: Marco Stornelli; +Cc: Linux FS Devel, Linux Kernel
On Sun, Sep 11, 2011 at 10:25:03AM +0200, Marco Stornelli wrote:
> Il 10/09/2011 17:56, Al Viro ha scritto:
> >On Sat, Sep 10, 2011 at 05:31:39PM +0200, Marco Stornelli wrote:
> >>From: Marco Stornelli<marco.stornelli@gmail.com>
> >>
> >>There aren't sufficient sync points for a fs for xip operations. In
> >>particular for the mmap case. It can be not sufficient to lock/unlock
> >>to do some operation inside get_xip_mem callback. For xip_file_read
> >>it's really easy to write a fs specific wrapper, xip_file_write hold
> >>i_mutex so no problem. With this patch we can avoid concurrent truncate
> >>operation and xip mmap.
> >
> >Umm... I really don't like that; what's going to happen if you have a file
> >mmapped and do write() to that file from address in that mapping?
> >
>
> Nothing strange. There is a serialization of the operations. Maybe I
> don't understand the point here.
write() grabs ->i_mutex on the file it's going to write to. It uses
copy_from_user() while holding ->i_mutex; that can end up calling ->fault().
If your data comes from the same file mmapped in your address space, you
have xip_write_fault() called while you are in xip_file_write() and *already*
are holding ->i_mutex on the same inode. With your patch it will, AFAICS,
cheerfully deadlock.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault
2011-09-11 11:15 ` Al Viro
@ 2011-09-11 11:25 ` Al Viro
2011-09-11 15:57 ` Marco Stornelli
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2011-09-11 11:25 UTC (permalink / raw)
To: Marco Stornelli; +Cc: Linux FS Devel, Linux Kernel
On Sun, Sep 11, 2011 at 12:15:04PM +0100, Al Viro wrote:
> write() grabs ->i_mutex on the file it's going to write to. It uses
> copy_from_user() while holding ->i_mutex; that can end up calling ->fault().
> If your data comes from the same file mmapped in your address space, you
> have xip_write_fault() called while you are in xip_file_write() and *already*
> are holding ->i_mutex on the same inode. With your patch it will, AFAICS,
> cheerfully deadlock.
Oh, wait... You are only doing that to write side of pagefault? That's
better, but not much:
thread 1: mmap the file, modify mapping
thread 2: write() to file
The former will do xip_write_fault() while holding ->mmap_sem.
The latter will do copy_from_user() from xip_file_write(), getting
pagefaults while holding ->i_mutex.
Note that we are grabbing ->mmap_sem and ->i_mutex in opposite orders.
I.e. that will deadlock on you - all you need is threads sharing the
address space.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault
2011-09-11 11:25 ` Al Viro
@ 2011-09-11 15:57 ` Marco Stornelli
0 siblings, 0 replies; 6+ messages in thread
From: Marco Stornelli @ 2011-09-11 15:57 UTC (permalink / raw)
To: Al Viro; +Cc: Linux FS Devel, Linux Kernel
Il 11/09/2011 13:25, Al Viro ha scritto:
> On Sun, Sep 11, 2011 at 12:15:04PM +0100, Al Viro wrote:
>
>> write() grabs ->i_mutex on the file it's going to write to. It uses
>> copy_from_user() while holding ->i_mutex; that can end up calling ->fault().
>> If your data comes from the same file mmapped in your address space, you
>> have xip_write_fault() called while you are in xip_file_write() and *already*
>> are holding ->i_mutex on the same inode. With your patch it will, AFAICS,
>> cheerfully deadlock.
>
> Oh, wait... You are only doing that to write side of pagefault? That's
> better, but not much:
>
> thread 1: mmap the file, modify mapping
> thread 2: write() to file
>
> The former will do xip_write_fault() while holding ->mmap_sem.
> The latter will do copy_from_user() from xip_file_write(), getting
> pagefaults while holding ->i_mutex.
>
> Note that we are grabbing ->mmap_sem and ->i_mutex in opposite orders.
> I.e. that will deadlock on you - all you need is threads sharing the
> address space.
>
Ok, thank you very much for the on-line debug :) So i_mutex is not a
good lock to use in this situation. It was a common sync point, but it
has some collateral effect on the write path that we must avoid. At this
point, what can be a good strategy? Any opinion is welcome.
Marco
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-11 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-10 15:31 [PATCH, RFC] xip: use i_mutex for xip_file_fault Marco Stornelli
2011-09-10 15:56 ` Al Viro
2011-09-11 8:25 ` Marco Stornelli
2011-09-11 11:15 ` Al Viro
2011-09-11 11:25 ` Al Viro
2011-09-11 15:57 ` Marco Stornelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).