On Thu, May 15, 2014 at 7:45 AM, Erez Zadok <[email protected]> wrote: > Absolutely. I already verified the bug on wrapfs first (easier to > test in a smaller template :-), then fixed it there; then did the same > to unionfs. Then back-ported to all previous versions applicable > (some older kernels took a struct page as 2nd arg of ->page_mkwrite). > And updated to latest kernels while I’m at it. All of this was > pushed to git.fsl.cs.sunysb.edu. See actual patch below for the > latest Linus tree.
This is the same patch I tested with and can confirm that I don't see the BUG_ON triggered with this patch. The other big issue, however, is the lack of direct_IO support in unionfs. Is it scheduled to be added? Is there any ETA if it's being worked on? Thanks Vaibhav > > Cheers, > Erez. > > On May 14, 2014, at 11:23 PM, Theodore Ts'o <[email protected]> wrote: > >> BTW, I assume it won't surprise you to discover that wrapfs has the >> same problem, right? > > diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c > index 28b6ce8..488a582 100644 (file) > --- a/fs/unionfs/mmap.c > +++ b/fs/unionfs/mmap.c > @@ -65,6 +65,41 @@ static int unionfs_fault(struct vm_area_struct *vma, > struct vm_fault *vmf) > return err; > } > > +static int unionfs_page_mkwrite(struct vm_area_struct *vma, > + struct vm_fault *vmf) > +{ > + int err = 0; > + struct file *file, *lower_file; > + const struct vm_operations_struct *lower_vm_ops; > + struct vm_area_struct lower_vma; > + > + BUG_ON(!vma); > + memcpy(&lower_vma, vma, sizeof(struct vm_area_struct)); > + file = lower_vma.vm_file; > + lower_vm_ops = UNIONFS_F(file)->lower_vm_ops; > + BUG_ON(!lower_vm_ops); > + if (!lower_vm_ops->page_mkwrite) > + goto out; > + > + lower_file = unionfs_lower_file(file); > + BUG_ON(!lower_file); > + /* > + * XXX: vm_ops->page_mkwrite may be called in parallel. > + * Because we have to resort to temporarily changing the > + * vma->vm_file to point to the lower file, a concurrent > + * invocation of unionfs_page_mkwrite could see a different > + * value. In this workaround, we keep a different copy of the > + * vma structure in our stack, so we never expose a different > + * value of the vma->vm_file called to us, even temporarily. > + * A better fix would be to change the calling semantics of > + * ->page_mkwrite to take an explicit file pointer. > + */ > + lower_vma.vm_file = lower_file; > + err = lower_vm_ops->page_mkwrite(&lower_vma, vmf); > +out: > + return err; > +} > + > /* > * XXX: the default address_space_ops for unionfs is empty. We cannot set > * our inode->i_mapping->a_ops to NULL because too many code paths expect > @@ -86,4 +121,5 @@ struct address_space_operations unionfs_dummy_aops = { > > struct vm_operations_struct unionfs_vm_ops = { > .fault = unionfs_fault, > + .page_mkwrite = unionfs_page_mkwrite, > }; > _______________________________________________ unionfs mailing list: http://unionfs.filesystems.org/ [email protected] http://www.fsl.cs.sunysb.edu/mailman/listinfo/unionfs
