On 2020/03/28 18:07, hch@infradead.org wrote:

On Sat, Mar 28, 2020 at 09:02:43AM +0000, Damien Le Moal wrote:

quoted

On 2020/03/28 17:51, Christoph Hellwig wrote:

quoted

quoted

Since zone reset and finish operations can be issued concurrently with
writes and zone append requests, ensure a coherent update of the zone
write pointer offsets by also write locking the target zones for these
zone management requests.

While they can be issued concurrently you can't expect sane behavior
in that case.  So I'm not sure why we need the zone write lock in this
case.

The behavior will certainly not be sane for the buggy application doing writes
and resets to the same zone concurrently (I have debugged that several time in
the field). So I am not worried about that at all. The zone write lock here is
still used to make sure the wp cache stays in sync with the drive. Without it,
we could have races on completion update of the wp and get out of sync.

How do the applications expect to get sane results from that in general?

They do not get sane results :) That's application bugs. I do not care about
those. What I do care is that the wp cache stays in sync with the drive so that
it itself does not become the cause of errors.

Rethinking about it though, the error processing code doing a zone report and wp
cache refresh will trigger for any write error, even those resulting from dumb
application bugs. So protection or not, since the wp cache refresh will be done,
we could simply no do zone write locking for reset and finish since these are
really expected to be done without any in-flight write.

But if you think protecting against that is worth the effort I think
there should be a separate patch to take the zone write lock for
reset/finish.

OK. That would be easy to add. But from the point above, I am now trying to
convince myself that this is not necessary.

quoted

quoted

quoted

+#define SD_ZBC_INVALID_WP_OFST	~(0u)
+#define SD_ZBC_UPDATING_WP_OFST	(SD_ZBC_INVALID_WP_OFST - 1)

Given that this goes into the seq_zones_wp_ofst shouldn't the block
layer define these values?

We could, at least the first one. The second one is really something that could
be considered completely driver dependent since other drivers doing this
emulation may handle the updating state differently.

Since this is the only driver where this is needed, may be we can keep this here
for now ?

Well, I'd rather keep magic values for a field defined in common code
in the common code.  Having behavior details spread over different
modules makes code rather hard to follow.

quoted

quoted

quoted

+struct sd_zbc_zone_work {
+	struct work_struct work;
+	struct scsi_disk *sdkp;
+	unsigned int zno;
+	char buf[SD_BUF_SIZE];
+};

Wouldn't it make sense to have one work_struct per scsi device and batch
updates?  That is also query a decenent sized buffer with a bunch of
zones and update them all at once?  Also given that the other write
pointer caching code is in the block layer, why is this in SCSI?

Again, because we thought this is driver dependent in the sense that other
drivers may want to handle invalid WP entries differently.

What sensible other strategy exists?  Nevermind that I hope we never
see another driver.  And as above - I really want to keep behavior
togetether instead of wiredly split over different code bases.  My
preference would still be to have it just in sd, but you gave some good
arguments for keeping it in the block layer.  Maybe we need to take a
deeper look and figure out a way to keep it isolated in SCSI.

OK. We can try again to see if we can keep all this WP caching in sd. The only
pain point is the revalidation as I explained before. Everything else would stay
pretty much the same and all be scsi specific. I will dig again to see what can
be done.

quoted

Also, I think that
one work struct per device may be an overkill. This is for error recovery and on
a normal healthy systems, write errors are rare.

I think it is less overkill than the dynamic allocation scheme with
the mempool and slab cache, that is why I suggested it.

Ah. OK. Good point.

-- 
Damien Le Moal
Western Digital Research