Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support

[Jürgen Groß <jgross@xxxxxxxx>]

On 14.05.20 11:50, Jürgen Groß wrote:
> On 14.05.20 09:59, Jan Beulich wrote:
> > On 08.05.2020 17:34, Juergen Gross wrote:
> > > +int hypfs_read_dir(const struct hypfs_entry *entry,
> > > +                   XEN_GUEST_HANDLE_PARAM(void) uaddr)
> > > +{
> > > +    const struct hypfs_entry_dir *d;
> > > +    const struct hypfs_entry *e;
> > > +    unsigned int size = entry->size;
> > > +
> > > +    d = container_of(entry, const struct hypfs_entry_dir, e);
> > > +
> > > +    list_for_each_entry ( e, &d->dirlist, list )
> > This function, in particular because of being non-static, makes
> > me wonder how, with add_entry() taking a lock, it can be safe
> > without any locking. Initially I thought the justification might
> > be because all adding of entries is an init-time-only thing, but
> > various involved functions aren't marked __init (and it is at
> > least not implausible that down the road we might see new
> > entries getting added during certain hotplug operations).
> >
> > I do realize that do_hypfs_op() takes the necessary read lock,
> > but then you're still building on the assumption that the
> > function is reachable through only that code path, despite
> > being non-static. An ASSERT() here would be the minimum I guess,
> > but with read locks now being recursive I don't see why you
> > couldn't read-lock here again.
> Right, will add the read-lock.
>
> > The same goes for other non-static functions, albeit things may
> > become more interesting for functions living on the
> > XEN_HYPFS_OP_write_contents path (because write locks aren't
> Adding an ASSERT() in this regard should be rather easy.
As the type specific read- and write-functions should only be called
through the generic read/write functions I think it is better to have
a percpu variable holding the current locking state and ASSERT() that
to match. This will be cheaper than nesting locks and I don't have to
worry about either write_lock nesting or making _is_write_locked_by_me()
an official interface.


Juergen