On Thu, 2025-12-04 at 14:33 -0500, Benjamin Marzinski wrote:
> On Wed, Dec 03, 2025 at 08:19:33AM -0800, Brian Bunker wrote:
> How about a simplifying the purge thread slightly. When the checker
> gets
> a path it is supposed to purge, it sets purge_path, just like now. I
> don't think we need to track purge_retries at all. When the purge
> loop
> is triggered, it locks the vecs lock, and finds a single path to
> purge,
> like now. The only info it needs to save is the udev device (with a
> new
> refcount taken like now) and a dup of pp->fd (You need this to
> guarantee
> that the devnode won't get reused if the device is deleted and a new
> one
> is re-added). You could also save the name, but
> udev_device_get_devnode() will get it for you. Like I mentioned
> above,
> you need to have a pthread cleanup handler that gets run to clean
> these
> up if they are set and the thread is cancelled, just like how the
> vecs
> lock is handled. Then purgeloop() clears pp->purge_path, drops the
> vecs
> lock, does the sysfs delete, and cleans up the purge_info by calling
> pthread_cleanup_pop(). Finally it loops back to reacquiring the vecs
> lock and finding the next path to delete. I don't really see a value
> in
> all the stats tracking. If it goes through all the paths, and doesn't
> find one in need of purging, it waits, just like now.
I'd go one step further and suggest an approach similar to what we do
in the uevent handler, for which we have a separate list of uevents
that shares no data with the main multipath threads.
Roughly like this:
enum purge_state {
PURGE_REQUESTED = 1,
PURGE_DONE,
PURGE_ERROR
};
struct path_to_purge {
struct list_head queue;
struct udev_device *udev; /* one ref taken while on the list
*/
int fd; /* dup of pp->fd */
enum purge_state state;
};
// global list_head for the queue of paths to be purged.
static LIST_HEAD(purge_queue);
If multipathd detects a path to purge in the checker thread, it
allocates and adds a path_to_purge entry with state PURGE_REQUESTED to
the queue and triggers the purge thread e.g. with a pthreads condition
variable.
The purge thread then walks through the list (without modifying the
list itself), sends a delete request to the kernel and atomically sets
the state field to PURGE_DONE when successful.
multipathd's checker thread walks the list next time and removes
entries that have state != PURGE_REQUESTED, closing the fd and unref-
ing the struct udev. IOW the resource allocation/freeing for this list
and its elements would be handled by the checker alone. The checker
would not immediately drop internal references to the path; this would
be handled by the remove uevent handler as usual.
The purge thread would never write to the list, except to the state
attribute of list members (which we could protect from being freed via
RCU).
This would make the purge thread almost an independent process.
Opinions?
Martin
