mbox series

[0/3] Fix zone write validation

Message ID 20210126050248.9077-1-dmitry.fomichev@wdc.com (mailing list archive)
Headers show
Series Fix zone write validation | expand

Message

Dmitry Fomichev Jan. 26, 2021, 5:02 a.m. UTC
These patches solve a few problems that exist in zoned Write
ans Zone Append validation code.

Dmitry Fomichev (3):
  hw/block/nvme: Check for zone boundary during append
  hw/block/nvme: Check zone state before checking boundaries
  hw/block/nvme: Add trace events for zone boundary violations

 hw/block/nvme.c       | 35 ++++++++++++++++++++---------------
 hw/block/trace-events |  3 +++
 2 files changed, 23 insertions(+), 15 deletions(-)

Comments

Klaus Jensen Jan. 26, 2021, 8:21 a.m. UTC | #1
On Jan 26 14:02, Dmitry Fomichev wrote:
> These patches solve a few problems that exist in zoned Write
> ans Zone Append validation code.
> 
> Dmitry Fomichev (3):
>   hw/block/nvme: Check for zone boundary during append
>   hw/block/nvme: Check zone state before checking boundaries
>   hw/block/nvme: Add trace events for zone boundary violations
> 
>  hw/block/nvme.c       | 35 ++++++++++++++++++++---------------
>  hw/block/trace-events |  3 +++
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 

I don't think there are any obvious benefits to this series over my fix
and since you didn't identify any functional issues with it, I'm
thinking we go with that.

My fix additionally removes setting ALBA in the CQE for regular writes
and bundles the endianness fix by changing the related logic in
do_write.

I have a couple of series in queue that also includes zoned writes and
there is no reason they have to indirectly deal with append. It's just
cleaner to move this special case closer to where it's used.
Klaus Jensen Jan. 26, 2021, 8:40 a.m. UTC | #2
On Jan 26 09:21, Klaus Jensen wrote:
> On Jan 26 14:02, Dmitry Fomichev wrote:
> > These patches solve a few problems that exist in zoned Write
> > ans Zone Append validation code.
> > 
> > Dmitry Fomichev (3):
> >   hw/block/nvme: Check for zone boundary during append
> >   hw/block/nvme: Check zone state before checking boundaries
> >   hw/block/nvme: Add trace events for zone boundary violations
> > 
> >  hw/block/nvme.c       | 35 ++++++++++++++++++++---------------
> >  hw/block/trace-events |  3 +++
> >  2 files changed, 23 insertions(+), 15 deletions(-)
> > 
> 
> I don't think there are any obvious benefits to this series over my fix
> and since you didn't identify any functional issues with it, I'm
> thinking we go with that.
> 
> My fix additionally removes setting ALBA in the CQE for regular writes
> and bundles the endianness fix by changing the related logic in
> do_write.
> 
> I have a couple of series in queue that also includes zoned writes and
> there is no reason they have to indirectly deal with append. It's just
> cleaner to move this special case closer to where it's used.

Keith,

I think this calls for your +1 tiebreaking special ability.
Keith Busch Jan. 27, 2021, 5:46 p.m. UTC | #3
On Tue, Jan 26, 2021 at 09:40:44AM +0100, Klaus Jensen wrote:
> On Jan 26 09:21, Klaus Jensen wrote:
> > On Jan 26 14:02, Dmitry Fomichev wrote:
> > > These patches solve a few problems that exist in zoned Write
> > > ans Zone Append validation code.
> > > 
> > > Dmitry Fomichev (3):
> > >   hw/block/nvme: Check for zone boundary during append
> > >   hw/block/nvme: Check zone state before checking boundaries
> > >   hw/block/nvme: Add trace events for zone boundary violations
> > > 
> > >  hw/block/nvme.c       | 35 ++++++++++++++++++++---------------
> > >  hw/block/trace-events |  3 +++
> > >  2 files changed, 23 insertions(+), 15 deletions(-)
> > > 
> > 
> > I don't think there are any obvious benefits to this series over my fix
> > and since you didn't identify any functional issues with it, I'm
> > thinking we go with that.
> > 
> > My fix additionally removes setting ALBA in the CQE for regular writes
> > and bundles the endianness fix by changing the related logic in
> > do_write.
> > 
> > I have a couple of series in queue that also includes zoned writes and
> > there is no reason they have to indirectly deal with append. It's just
> > cleaner to move this special case closer to where it's used.
> 
> Keith,
> 
> I think this calls for your +1 tiebreaking special ability.

Klaus,

Your series arrived very timely, and since it looks fine (unless there
are outstanding objections?), I think it's fair to apply your series.
Subsequent changes can rebase to that.