Hi,
<big snip>
+ if (err)
+ return err;
+
+ *off += nwritten;
+ if (*off > inode->i_size)
+ i_size_write(inode, *off);
+
+ /* Invalidate page-cache so that mmap using apps see the changes too */
+ invalidate_mapping_pages(inode->i_mapping, pos >> PAGE_SHIFT,
+ *off >> PAGE_SHIFT);
+
+ /* mtime changed */
+ sf_i->force_restat = 1;
+ return nwritten;
+}
+ /* Already open? */
+ if (sf_i->handle != SHFL_HANDLE_NIL) {
+ sf_r->handle = sf_i->handle;
+ sf_i->handle = SHFL_HANDLE_NIL;
+ sf_i->file = file;
+ file->private_data = sf_r;
Obviously racy.
Ok, so how do we fix this? Add a mutex to the private
struct sf_inode_info and take that here and in other
relevant places I guess?
+ /* the host may have given us different attr then requested */
+ sf_i->force_restat = 1;
+ sf_r->handle = params.handle;
+ sf_i->file = file;
Ditto. Two opens in parallel and you are in trouble.
I assume what you fall over there is the "sf_i->file = file" which
is a hack which is purely implemented to have a handle for writepage.
Looking at this closer, I think the right thing todo might be to
implement our own .mmap callback + vma_close callback and then
refcount the handle and store it in vm_private_data so that we
can get it from there rather then having this hack of storing
a struct file * in the inode which as you rightly point out is
obviously buggy.
What do you think of using the vm_private_data approach?
Ok this is of course nonsense as we do not have access to the vma
from the writepage callback. So instead I plan to store a list
of writeable handles in the inode combined with a custom vma_close
like fuse is doing to make sure we dump any changes before loosing
the last handle.
Does that sound ok?
Regards,
Hans
