[PATCH] zbd: Restore check_swd()

[PATCH] zbd: Restore check_swd()

[Damien Le Moal]

As check_swd() is useful for debugging, revert its removal done with
commit d60be7d51cbb ("zbd: Remove unused function and variable").

Fixes: d60be7d51cbb ("zbd: Remove unused function and variable")
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 zbd.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/zbd.c b/zbd.c
index 9c525875..0b7159a0 100644
--- a/zbd.c
+++ b/zbd.c
@@ -726,6 +726,32 @@ static bool zbd_dec_and_reset_write_cnt(const struct thread_data *td,
 	return write_cnt == 0;
 }
 
+/*
+ * For debugging:
+ * Check whether the value of zbd_info.sectors_with_data is correct.
+ */
+static void check_swd(const struct thread_data *td, const struct fio_file *f)
+{
+#if 0
+	struct fio_zone_info *zb, *ze, *z;
+	uint64_t swd;
+
+	zb = &f->zbd_info->zone_info[zbd_zone_idx(f, f->file_offset)];
+	ze = &f->zbd_info->zone_info[zbd_zone_idx(f, f->file_offset +
+						  f->io_size)];
+	swd = 0;
+	for (z = zb; z < ze; z++) {
+		pthread_mutex_lock(&z->mutex);
+		swd += z->wp - z->start;
+	}
+	pthread_mutex_lock(&f->zbd_info->mutex);
+	assert(f->zbd_info->sectors_with_data == swd);
+	pthread_mutex_unlock(&f->zbd_info->mutex);
+	for (z = zb; z < ze; z++)
+		pthread_mutex_unlock(&z->mutex);
+#endif
+}
+
 void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 {
 	struct fio_zone_info *zb, *ze, *z;
@@ -1202,6 +1228,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 		}
 		/* Check whether the zone reset threshold has been exceeded */
 		if (td->o.zrf.u.f) {
+			check_swd(td, f);
 			if (f->zbd_info->sectors_with_data >=
 			    f->io_size * td->o.zrt.u.f &&
 			    zbd_dec_and_reset_write_cnt(td, f)) {
@@ -1222,6 +1249,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			zb->reset_zone = 0;
 			if (zbd_reset_zone(td, f, zb) < 0)
 				goto eof;
+			check_swd(td, f);
 		}
 		/* Make writes occur at the write pointer */
 		assert(!zbd_zone_full(f, zb, min_bs));
-- 
2.17.1

Re: [PATCH] zbd: Restore check_swd()

[Bart Van Assche]

On 9/26/18 7:16 PM, Damien Le Moal wrote:
> As check_swd() is useful for debugging, revert its removal done with
> commit d60be7d51cbb ("zbd: Remove unused function and variable").

Thanks!

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] zbd: Restore check_swd()

[Jens Axboe]

On 9/26/18 8:16 PM, Damien Le Moal wrote:
> As check_swd() is useful for debugging, revert its removal done with
> commit d60be7d51cbb ("zbd: Remove unused function and variable").

The problem with dead code is that it's often forgotten when
other changes are made, and then it doesn't even compile or
work anymore.

I'd be happier if this was dependent on some debug static
bool instead, ala:

static bool zbd_debug;

And then have check_swd() do

if (!zbd_debug)
    return;

instead of hiding everything behind an #if 0.

-- 
Jens Axboe

Re: [PATCH] zbd: Restore check_swd()

[Damien Le Moal]

Jens,

On 2018/09/28 12:53, Jens Axboe wrote:
> On 9/26/18 8:16 PM, Damien Le Moal wrote:
>> As check_swd() is useful for debugging, revert its removal done with
>> commit d60be7d51cbb ("zbd: Remove unused function and variable").
> 
> The problem with dead code is that it's often forgotten when
> other changes are made, and then it doesn't even compile or
> work anymore.
> 
> I'd be happier if this was dependent on some debug static
> bool instead, ala:
> 
> static bool zbd_debug;
> 
> And then have check_swd() do
> 
> if (!zbd_debug)
>     return;
> 
> instead of hiding everything behind an #if 0.

OK. No problem. I will update and send a v2.

(and another patch to fix the incorrect comments signaled by Bart)

Thanks !


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] zbd: Restore check_swd()

[Damien Le Moal]

Jens,

On 2018/09/28 13:04, Damien Le Moal wrote:
> Jens,
> 
> On 2018/09/28 12:53, Jens Axboe wrote:
>> On 9/26/18 8:16 PM, Damien Le Moal wrote:
>>> As check_swd() is useful for debugging, revert its removal done with
>>> commit d60be7d51cbb ("zbd: Remove unused function and variable").
>>
>> The problem with dead code is that it's often forgotten when
>> other changes are made, and then it doesn't even compile or
>> work anymore.
>>
>> I'd be happier if this was dependent on some debug static
>> bool instead, ala:
>>
>> static bool zbd_debug;
>>
>> And then have check_swd() do
>>
>> if (!zbd_debug)
>>     return;
>>
>> instead of hiding everything behind an #if 0.
> 
> OK. No problem. I will update and send a v2.
> 
> (and another patch to fix the incorrect comments signaled by Bart)
> 
> Thanks !

Not sending a fix after all: with check_swd() enabled, I am getting a deadlock
between check_swd() and zbd_file_reset() on a zone mutex in test 29 when running
against regular nullb.

That needs to be investigated. But I do not have the time to do so right away.

Bart,

If you can, please also look at it ? I do not see anything obviously wrong. It
may be a very subtle race.

Best regards.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] zbd: Restore check_swd()

[Jens Axboe]

On 9/27/18 11:48 PM, Damien Le Moal wrote:
> Jens,
> 
> On 2018/09/28 13:04, Damien Le Moal wrote:
>> Jens,
>>
>> On 2018/09/28 12:53, Jens Axboe wrote:
>>> On 9/26/18 8:16 PM, Damien Le Moal wrote:
>>>> As check_swd() is useful for debugging, revert its removal done with
>>>> commit d60be7d51cbb ("zbd: Remove unused function and variable").
>>>
>>> The problem with dead code is that it's often forgotten when
>>> other changes are made, and then it doesn't even compile or
>>> work anymore.
>>>
>>> I'd be happier if this was dependent on some debug static
>>> bool instead, ala:
>>>
>>> static bool zbd_debug;
>>>
>>> And then have check_swd() do
>>>
>>> if (!zbd_debug)
>>>     return;
>>>
>>> instead of hiding everything behind an #if 0.
>>
>> OK. No problem. I will update and send a v2.
>>
>> (and another patch to fix the incorrect comments signaled by Bart)
>>
>> Thanks !
> 
> Not sending a fix after all: with check_swd() enabled, I am getting a deadlock
> between check_swd() and zbd_file_reset() on a zone mutex in test 29 when running
> against regular nullb.
> 
> That needs to be investigated. But I do not have the time to do so right away.
> 
> Bart,
> 
> If you can, please also look at it ? I do not see anything obviously wrong. It
> may be a very subtle race.

+	for (z = zb; z < ze; z++) {
+		pthread_mutex_lock(&z->mutex);
+		swd += z->wp - z->start;
+	}
+	pthread_mutex_lock(&f->zbd_info->mutex);

Can 'z' ever end up being f->zbd_info? That would surely explain the
deadlock.

-- 
Jens Axboe

Re: [PATCH] zbd: Restore check_swd()

[Bart Van Assche]

On Fri, 2018-09-28 at 08:54 -0600, Jens Axboe wrote:
> +	for (z = zb; z < ze; z++) {
> +		pthread_mutex_lock(&z->mutex);
> +		swd += z->wp - z->start;
> +	}
> +	pthread_mutex_lock(&f->zbd_info->mutex);
> 
> Can 'z' ever end up being f->zbd_info? That would surely explain the
> deadlock.

Hi Jens,

f->zbd_info and z have a different type so unless something very weird is going
on &z->mutex can't be identical to &f->zbd_info->mutex.

What I think is going on is traditional lock inversion: check_swd() triggers
lock inversion if it is called while any of the z->mutex objects are held. One
such mutex is held where Damien tried to restore the calls of this function. I
will submit a patch that inserts these calls elsewhere.

Bart.

Re: [PATCH] zbd: Restore check_swd()

[Jens Axboe]

On 9/28/18 2:25 PM, Bart Van Assche wrote:
> On Fri, 2018-09-28 at 08:54 -0600, Jens Axboe wrote:
>> +	for (z = zb; z < ze; z++) {
>> +		pthread_mutex_lock(&z->mutex);
>> +		swd += z->wp - z->start;
>> +	}
>> +	pthread_mutex_lock(&f->zbd_info->mutex);
>>
>> Can 'z' ever end up being f->zbd_info? That would surely explain the
>> deadlock.
> 
> Hi Jens,
> 
> f->zbd_info and z have a different type so unless something very weird is going
> on &z->mutex can't be identical to &f->zbd_info->mutex.

Obviously I didn't look too closely :-)

> What I think is going on is traditional lock inversion: check_swd() triggers
> lock inversion if it is called while any of the z->mutex objects are held. One
> such mutex is held where Damien tried to restore the calls of this function. I
> will submit a patch that inserts these calls elsewhere.

That makes sense.

-- 
Jens Axboe