[dm-devel] [PATCH] libmultipath: simplify failed wwid code

[Martin Wilck]

On Fri, 2020-05-01 at 17:39 -0500, Benjamin Marzinski wrote:
> The (is|mark|unmark)_failed_wwid code is needlessly complicated.
> Locking a file is necssary if multiple processes could otherwise be
> writing to it at the same time. That is not the case with the
> failed_wwids files. They can simply be empty files in a
> directory.  Even
> with all the locking in place, two processes accessing or modifying a
> file at the same time will still race. And even without the locking,
> if
> two processes try to access or modify a file at the same time, they
> will
> both see a reasonable result, and will leave the files in a valid
> state
> afterwards.
> 
> Instead of doing all the locking work (which made it necessary to
> write
> a file, even just to check if a file existed), simply check for files
> with lstat(), create them with open(), and remove them with unlink().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  libmultipath/wwids.c | 131 ++++++++++++++++++-----------------------
> --
>  1 file changed, 56 insertions(+), 75 deletions(-)

I agree, almost :-)

Please consider adding the attached patch on top, which switches back
to atomic creation of the failed_wwids file, with just a little bit
more compexity. I prefer the creation of the file to be atomic on the
file system level. IMO that's how "flag" files like this should be
handled in principle, and doing it correctly makes me feel better, even
though I have to concur that an actual race is hardly possible.

Regards,
Martin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libmultipath-use-atomic-linkat-in-mark_failed_wwid.patch
Type: text/x-patch
Size: 2840 bytes
Desc: 0001-libmultipath-use-atomic-linkat-in-mark_failed_wwid.patch
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20200508/3603943c/attachment.bin>