On Thu, 2025-02-27 at 11:50 +0000, David Howells wrote:
> Viacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
>
> > This patch fixes the issue by means of locking
> > client->osdc.lock and client->monc.mutex before
> > the checking client->osdc.osdmap and
> > client->monc.monmap.
>
> You've also added clearance of a bunch of pointers into destruction and error
> handling paths (can I recommend you mention it in the commit message?).
>
Yes, makes sense. I missed to add these details into the commit message. I can
do it.
> Is
> that a "just in case" thing? It doesn't look like the client can get
> resurrected afterwards, but I may have missed something. If it's not just in
> case, does the access and clearance of the pointers need wrapping in the
> appropriate lock?
>
Let me double check it. Probably, you are right here.
> > --- a/net/ceph/debugfs.c
> > +++ b/net/ceph/debugfs.c
> > @@ -36,18 +36,20 @@ static int monmap_show(struct seq_file *s, void *p)
> > int i;
> > struct ceph_client *client = s->private;
> >
> > - if (client->monc.monmap == NULL)
> > - return 0;
> > -
> > - seq_printf(s, "epoch %d\n", client->monc.monmap->epoch);
> > - for (i = 0; i < client->monc.monmap->num_mon; i++) {
> > - struct ceph_entity_inst *inst =
> > - &client->monc.monmap->mon_inst[i];
> > -
> > - seq_printf(s, "\t%s%lld\t%s\n",
> > - ENTITY_NAME(inst->name),
> > - ceph_pr_addr(&inst->addr));
> > + mutex_lock(&client->monc.mutex);
> > + if (client->monc.monmap) {
> > + seq_printf(s, "epoch %d\n", client->monc.monmap->epoch);
> > + for (i = 0; i < client->monc.monmap->num_mon; i++) {
> > + struct ceph_entity_inst *inst =
> > + &client->monc.monmap->mon_inst[i];
> > +
> > + seq_printf(s, "\t%s%lld\t%s\n",
> > + ENTITY_NAME(inst->name),
> > + ceph_pr_addr(&inst->addr));
> > + }
> > }
> > + mutex_unlock(&client->monc.mutex);
> > +
> > return 0;
> > }
> >
>
> You might want to look at using RCU for this (though not necessarily as part
> of this fix).
>
Potentially, RCU could be good to use here. Let me think about it. I simply need
to elaborate a balance of modifications and potential benefits.
Thanks,
Slava.
