From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Cc: "fio@vger.kernel.org" <fio@vger.kernel.org>,
Jens Axboe <axboe@kernel.dk>, Vincent Fu <vincentfu@gmail.com>,
Damien Le Moal <dlemoal@kernel.org>,
Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
Subject: Re: [PATCH 4/7] zbd: fix write zone accounting of trim workload
Date: Thu, 8 Jun 2023 08:10:08 +0000 [thread overview]
Message-ID: <ZIGM1waG1/wgawP+@x1-carbon> (raw)
In-Reply-To: <gp4w4ente632nsyp7llblq3hjbnnpfsw2drm4s3vbhfkrrfq5a@ok2icgcyievr>
On Thu, Jun 08, 2023 at 05:48:41AM +0000, Shinichiro Kawasaki wrote:
> On Jun 07, 2023 / 13:15, Niklas Cassel wrote:
> > On Wed, Jun 07, 2023 at 05:32:27PM +0900, Shin'ichiro Kawasaki wrote:
(snip)
> > > @@ -404,9 +431,6 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
> > > continue;
> > >
> > > zone_lock(td, f, z);
> > > - pthread_mutex_lock(&f->zbd_info->mutex);
> > > - zbd_write_zone_put(td, f, z);
> > > - pthread_mutex_unlock(&f->zbd_info->mutex);
> > >
> > > if (z->wp != z->start) {
> > > dprint(FD_ZBD, "%s: resetting zone %u\n",
> > > @@ -2048,7 +2072,7 @@ retry:
> > > */
> > > io_u_quiesce(td);
> > > zb->reset_zone = 0;
> > > - if (zbd_reset_zone(td, f, zb) < 0)
> > > + if (__zbd_reset_zone(td, f, zb) < 0)
> >
> > Since you remove the zbd_write_zone_put() call, which I think is good,
> > I think that you should continue to call zbd_reset_zone() here.
>
> The two hunks above are difficult for review. The 1st hunk which removes the
> zbd_write_zone_put() is a change for zbd_reset_zones(). zbd_reset_zones() no
> longer needs to call zbd_write_zone_put(), since zbd_reset_zone() (no s) calls
> zbd_write_zone_put().
>
> The 2nd hunk is a change in zbd_adjust_block(). The zone reset here is done
> just before the write command issue to the zone, so zbd_write_zone_put() should
> not be called. Then zbd_reset_zone() should be replaced with __zbd_reset_zone().
You are absolutely correct!
I confused the hunk at:
@@ retry:
For being for zbd_reset_zones(), but it really is for zbd_adjust_block().
(And zbd_reset_zones() still calls zbd_reset_zone() as it should.)
I really hate the patch format for using labels instead the actual function
name... Especially since you can have the same label in different functions.
Sorry for the noise.
Kind regards,
Niklas
next prev parent reply other threads:[~2023-06-08 8:10 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-07 8:32 [PATCH 0/7] zbd: clean up code and fix bugs for open zones accounting Shin'ichiro Kawasaki
2023-06-07 8:32 ` [PATCH 1/7] zbd: rename 'open zones' to 'write zones' Shin'ichiro Kawasaki
2023-06-07 13:15 ` Niklas Cassel
2023-06-07 8:32 ` [PATCH 2/7] zbd: do not reset extra zones in open conditions Shin'ichiro Kawasaki
2023-06-07 13:15 ` Niklas Cassel
2023-06-07 8:32 ` [PATCH 3/7] zbd: fix write zone accounting of almost full zones Shin'ichiro Kawasaki
2023-06-07 13:15 ` Niklas Cassel
2023-06-07 8:32 ` [PATCH 4/7] zbd: fix write zone accounting of trim workload Shin'ichiro Kawasaki
2023-06-07 13:15 ` Niklas Cassel
2023-06-08 5:48 ` Shinichiro Kawasaki
2023-06-08 8:10 ` Niklas Cassel [this message]
2023-06-07 15:32 ` Jens Axboe
2023-06-08 5:49 ` Shinichiro Kawasaki
2023-06-07 8:32 ` [PATCH 5/7] t/zbd: reset zones before tests with max_open_zones option Shin'ichiro Kawasaki
2023-06-07 8:32 ` [PATCH 6/7] t/zbd: test write zone accounting of almost full zones Shin'ichiro Kawasaki
2023-06-07 8:32 ` [PATCH 7/7] t/zbd: test write zone accounting of trim workload Shin'ichiro Kawasaki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZIGM1waG1/wgawP+@x1-carbon \
--to=niklas.cassel@wdc.com \
--cc=Dmitry.Fomichev@wdc.com \
--cc=axboe@kernel.dk \
--cc=dlemoal@kernel.org \
--cc=fio@vger.kernel.org \
--cc=shinichiro.kawasaki@wdc.com \
--cc=vincentfu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).