On Mon, Oct 7, 2024 at 12:38 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Mon, 7 Oct 2024 at 12:22, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > Maybe it is more straightforward, I can go with that, but it
> > feels like a waste not to use the space in backing_file,
> > so let me first try to convince you otherwise.
>
> Is it not a much bigger waste to allocate backing_file with kmalloc()
> instead of kmem_cache_alloc()?
Yes, much bigger...
Christian is still moving things around wrt lifetime of file and
backing_file, so I do not want to intervene in the file_table.c space.
>
> > IMO, this is not a layer violation at all.
> > The way I perceive struct backing_file is as an inheritance from struct file,
> > similar to the way that ovl_inode is an inheritance from vfs_inode.
>
> That sounds about right.
>
> > You can say that backing_file_user_path() is the layer violation, having
> > the vfs peek into the ovl layer above it, but backing_file_private_ptr()
> > is the opposite - it is used only by the layer that allocated backing_file,
> > so it is just like saying that a struct file has a single private_data, while
> > the inherited generic backing_file can store two private_data pointers.
> >
> > What's wrong with that?
>
> It feels wrong to me, because lowerfile's backing_file is just a
> convenient place to stash a completely unrelated pointer into.
Funny, that's like saying that a ->next member in a struct is
completely unrelated.
What I see after my patch is that ->private_data points to a singly
linked list of length 1 to 2 of backing files.
>
> Furthermore private_data pointers lack type safety with all the
> problems that entails.
Well, this is not any worth that current ->private_data, but I could
also make it, if you like it better:
struct backing_file {
struct file file;
struct path user_path;
+ struct file *next;
};
+struct file **backing_file_private_ptr(struct file *f)
+{
+ return &backing_file(f)->next;
+}
+EXPORT_SYMBOL_GPL(backing_file_next_ptr);
Again, I am not terribly opposed to allocating struct ovl_file as we do
with directory - it is certainly more straight forward to read, so that
is a good enough argument in itself, and "personal dislike" is also a fair
argument, just arguing for the sake of argument so you understand my POV.
Let me know your decision.
Thanks,
Amir.
