Re: [PATCH 0/7] Add data to datalog, add replay tool

[Wouter Verhelst <w@uter.be>]

Hi Manfred,

On Thu, Jan 13, 2022 at 08:34:00AM +0100, Manfred Spraul wrote:
> Hi,
> 
> As announced, here is the initial series for adding a replay tool.
> 
> Patch 1: Add support for pretty-printing WRITE_ZEROS and TRIM to
>         nbd-trdump. I've added a 'static inline' helper function to a
>         new header file.
> 	Is the approacch good? As alternative, I could add the function
> 	to e.g. cliserv.{c,h}. Would that be preferred?

It might make sense to add it there, but I have no strong preference
either way.

> Patch 2: Add actual data logging to nbd-server
> 	I've defined a new flag to indicate that actual data will follow.
> 	Any better ideas?

This changing of the protocol format for on-disk logging rubs me the
wrong way. I don't think that's the best way forward.

I think adding a header to the file to describe its format is a better
idea. If it started with a magic number that is different from any of
the NBD magic numbers (but is in the same location), then nbd-trdump
(and your new nbd-trplay) could switch on that to decide whether to
read/parse the header.

> Patch 3: Add locking.
> 	Is sem_open() portable enough? Should I add a few fallbacks?

It seems like it's defined by POSIX.1-2001? Should be fine.

This will slow things down somewhat if you're using the transaction log,
but we can document that easily (and it's meant as a debugging/hacking
tool, anyway).

> Patch 4: Add logging of the replies to nbd-server.
> 
> Patch 5: Add a new tool nbd-trplay
> 	Did I update Makefile.am correctly?

Yes.

> Patch 6: Initial CLI.
> Patch 7: Actual implementation.

Could you add a man page as well?

> I've used the code a bit, and it seems that it finds at least
> one suspicious case with ext4:
> https://lore.kernel.org/all/baa3101d-e2f7-823e-040f-8739ab610419@colorfullife.com/

Interesting :)

-- 
     w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}