Re: [PATCH v2] xfs: tear down zoned sysfs before freeing zone info

[Cen Zhang]

Hi Damien,

Thanks for your review and suggestions.

> Hmmm... What if the user already has entered the show function but gets
> preempted before referencing the zone info and umount proceeds ? That problem
> remains, no ?

I double checked the sysfs/kernfs paths, and I do not think that specific
case remains with the current ordering.  sysfs reads go through the kernfs
seq_file path: kernfs_seq_start() takes an active reference before
sysfs_kf_seq_show() calls the attribute's ->show method, and
kernfs_seq_stop() drops it afterwards.  On removal, kobject_del() reaches
sysfs_remove_dir(), which calls kernfs_remove(); kernfs_remove() deactivates
the subtree and kernfs_drain() waits for active references to drain.

So if nr_open_zones_show() has already been entered and then gets preempted,
xfs_zoned_sysfs_del() should wait for that active callback to finish before
returning.  Only after that would xfs_unmount_zones() free m_zone_info.

> Why not call this in xfs_mount_zones() ?

Yes, that is cleaner.  The zoned sysfs files expose zone allocator state, so
putting xfs_zoned_sysfs_init() in xfs_mount_zones() makes the ownership and
lifetime boundary more obvious.  I will move it there for v3.

> And this as the first thing in xfs_unmount_zones() ?

Agreed.  I will make xfs_unmount_zones() remove the zoned sysfs directory
before zone GC teardown and before xfs_free_zone_info().  That keeps all
zone-info users behind the same allocator teardown entry point.

One detail is that if xfs_zoned_sysfs_init() is called after
xfs_zone_gc_mount() and then fails, the mount error path in xfs_mount_zones()
must unwind the GC thread before freeing m_zone_info.  I will handle that in
the v3 error path.

> Another simpler approach could be to simply call xfs_mount_sysfs_del() earlier
> in xfs_unmountfs(). Though that may have other implications and I have not
> checked details.

I would rather avoid moving the whole xfs_mount_sysfs_del() earlier.  The
bug is specific to the zoned sysfs subtree because those files expose
m_zone_info, whereas the rest of the per-mount sysfs tree has different
state and lifetime.  Moving only the zoned subtree into
xfs_mount_zones()/xfs_unmount_zones() seems narrower and keeps unrelated
sysfs teardown ordering unchanged.

Best Regards,
Cen Zhang