Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Feb 2021 11:23:37 -0500
From:   David Jeffery <djeffery@redhat.com>
To:     Ming Lei <ming.lei@redhat.com>
Cc:     linux-block@vger.kernel.org, Jens Axboe <axboe@kernel.dk>,
        linux-kernel@vger.kernel.org,
        Laurence Oberman <loberman@redhat.com>
Subject: Re: [PATCH] block: recalculate segment count for multi-segment
 discard requests correctly
Message-ID: <20210203162337.GA40163@redhat>
References: <20210201164850.391332-1-djeffery@redhat.com>
 <20210202033343.GA165584@T590>
 <20210202204355.GA31803@redhat>
 <20210203023517.GA948998@T590>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210203023517.GA948998@T590>
Precedence: bulk

On Wed, Feb 03, 2021 at 10:35:17AM +0800, Ming Lei wrote:
> 
> On Tue, Feb 02, 2021 at 03:43:55PM -0500, David Jeffery wrote:
> > The return 0 does seem to be an old relic that does not make sense anymore.
> > Moving REQ_OP_SECURE_ERASE to be with discard and removing the old return 0,
> > is this what you had in mind?
> > 
> >  
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index 808768f6b174..68458aa01b05 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -383,8 +383,14 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
> >  	switch (bio_op(rq->bio)) {
> >  	case REQ_OP_DISCARD:
> >  	case REQ_OP_SECURE_ERASE:
> > +		if (queue_max_discard_segments(rq->q) > 1) {
> > +			struct bio *bio = rq->bio;
> > +			for_each_bio(bio)
> > +				nr_phys_segs++;
> > +			return nr_phys_segs;
> > +		}
> > +		/* fall through */
> >  	case REQ_OP_WRITE_ZEROES:
> > -		return 0;
> >  	case REQ_OP_WRITE_SAME:
> >  		return 1;
> 
> WRITE_SAME uses same buffer, so the nr_segment is still one; WRITE_ZERO
> doesn't need extra payload, so nr_segments is zero, see
> blk_bio_write_zeroes_split(), blk_bio_write_same_split, attempt_merge()
> and blk_rq_merge_ok().
> 

I thought you mentioned virtio-blk because of how some drivers handle
zeroing and discarding similarly and wanted to align the segment count with
discard behavior for WRITE_ZEROES too. (Though that would also need an update
to blk_bio_write_zeroes_split as you pointed out.)  So you want me to leave
WRITE_ZEROES behavior alone and let blk_rq_nr_discard_segments() keep doing
the hiding of a 0 rq->nr_phys_segments as 1 segment in the WRITE_ZEROES treated
as a discard case?

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..756473295f19 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -383,6 +383,14 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
 	case REQ_OP_SECURE_ERASE:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		return 1;
 	case REQ_OP_WRITE_ZEROES:
 		return 0;
 	case REQ_OP_WRITE_SAME:

--
David Jeffery