[PATCH RFC 2/4] erofs: introduce page cache share feature

[Amir Goldstein]

> > +}
> > +
> > +/*
> > + * TODO: Hm, could we leverage our fancy new backing file infrastructure
> > + * as for overlayfs and fuse?
>
> If some code can be lifted up as a vfs helper, it would be
> much helpful as the backing file infrastructure was lifted
> from overlayfs.
>
> But I'm not sure if it's really needed for now anyway
> because it's only EROFS-specific, and I only maintain and
> can speak of EROFS.
>
> > + */
> > +static struct file *erofs_pcs_alloc_file(struct file *file,
> > +                                      struct inode *ano_inode)
> > +{
> > +     struct file *ano_file;
> > +
> > +     ano_file = alloc_file_pseudo(ano_inode, erofs_pcs_mnt, "[erofs_pcs_f]",
> > +                                  O_RDONLY, &erofs_file_fops);
> > +     file_ra_state_init(&ano_file->f_ra, file->f_mapping);
> > +     ano_file->private_data = EROFS_I(file_inode(file));
> > +     return ano_file;
> > +}
> > +
>
> ...
>
> > +
> > +/*
> > + * TODO: Amir, you've got some experience in this area due to overlayfs
> > + * and fuse. Does that work?
> > + */
>
>
>
> Hi Amir,
>
> I do think it will work, if you have chance please help
> take a quick look too.
>
> It's much similar to overlayfs, the difference is that the real
> inodes is not in some other fs, but anon inodes from a pseudo
> sb which shares among the whole host to share page cache for
> containers.
>

I will answer that from two different POV.

1. Will the backing_file helpers reduce much complicated code?

Not really IMO, because EROFS shared inode does not support
dio/aio and does not require override_cred(), so the remaining bit
of read_ite/splice_read/mmap are pretty trivial IMO.

2. When is it ok to use the backing_file helpers?

The current patch allocates a struct file with
alloc_file_pseudo() and not a struct backing_file,
so using the backing_file helpers is going to be awkward and
misleading and I think in this case it will we wize to refrain
from using a local var name backing_file.

The thing that you need to ask yourself is do you want
backing_file_set_user_path() for the erofs shared inode.

That means, what do you want users to see when they
look at /proc/self/map_files symlinks.

With the current patch set I believe they will see a symlink to
something like "[erofs_pcs_f]" for any mapped file
which is somewhat odd.

I think it would have been nice if users saw something like
"[erofs_pcs_md5digest:XXXXXXX]"
IMO, making the page cache dedupe visible in map_files is
a very nice feature.

Overlayfs took the approach that users are expecting to see
the actual path of the (non-anonymous) file that they mapped
when looking at map_files symlinks.
If you do not display the path to erofs mount in map_files,
then lsof will not be able to blame a process with mapped files
as the reason for keeping erofs mount busy.

Thanks,
Amir.