On Thu, Feb 13, 2020 at 9:12 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Mon, Feb 10, 2020 at 4:11 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
>
> > static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
> > {
> > struct file *realfile = file->private_data;
> > const struct cred *old_cred;
> > + struct inode *inode = file->f_inode;
> > + struct ovl_copy_up_work ovl_cuw;
> > + DEFINE_WAIT_BIT(wait, &ovl_cuw.flags, OVL_COPY_UP_PENDING);
> > + wait_queue_head_t *wqh;
> > int ret;
> >
> > + if (vma->vm_flags & MAP_SHARED &&
> > + ovl_copy_up_shared(file_inode(file)->i_sb)) {
> > + ovl_cuw.err = 0;
> > + ovl_cuw.flags = 0;
> > + ovl_cuw.dentry = file_dentry(file);
> > + set_bit(OVL_COPY_UP_PENDING, &ovl_cuw.flags);
> > +
> > + wqh = bit_waitqueue(&ovl_cuw.flags, OVL_COPY_UP_PENDING);
> > + prepare_to_wait(wqh, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
> > +
> > + INIT_WORK(&ovl_cuw.work, ovl_copy_up_work_fn);
> > + schedule_work(&ovl_cuw.work);
> > +
> > + schedule();
> > + finish_wait(wqh, &wait.wq_entry);
>
> This just hides the bad lock dependency, it does not remove it.
>
> The solution we've come up with is arguably more complex, but it does
> fix this properly: make overlay use its own address space operations
> in case of a shared map.
>
> Amir, I lost track, do you remember what's the status of this work?
>
I'm afraid it is still standing at the side of the road where we left it...
I haven't had any time to work on it since.
The latest WIP branch is at:
https://github.com/amir73il/linux/commits/ovl-aops-wip
And summary of what it contains is at:
https://lore.kernel.org/linux-unionfs/CAJfpegsyA4SjmtAEpkMoKsvgmW0CiEwWEAbU7v3yJztLKmC0Eg@xxxxxxxxxxxxxx/
Problem is, this WIP doesn't even solve the MAP_SHARED case yet,
but it is a big step in the direction of the design you laid out here:
https://lore.kernel.org/linux-unionfs/CAJfpegvJU32_9_mVh7kem0s529-8Qs02fPSr4ChCC3ZJ2pRhLw@xxxxxxxxxxxxxx/
Chengguang,
If you are up for the task, feel free to pick up the WIP branch
and bring it into shape for merging.
Then we can also discuss the next steps for fixing MAP_SHARED.
BTW, you did not mention why MAP_SHARED case is important
in your workload. I'm just curious how important is it to solve it.
Thanks,
Amir.
