On Mon, Feb 20 2012 at 10:57pm -0500, Martin K. Petersen <martin.petersen@xxxxxxxxxx> wrote: > >>>>> "Mike" == Mike Snitzer <snitzer@xxxxxxxxxx> writes: > > Mike, > > Mike> One, thing I noticed: bio_has_data returns false for > Mike> REQ_WRITE_SAME. But REQ_WRITE_SAME does have data, and it really > Mike> should be accounted no?: > > I decided against it. We don't count discards either and write sames are > not really page-out types of activity. Happy to change it if people > think this is something we should handle. But what do we actually count? > A single logical block or the number of sectors written by the target > device? I'd say the single logical block. > Mike> That aside, I tried your updated code and hit this BUG when I use > Mike> the patch that has always worked (my dm-thin patch that uses the > Mike> blkdev_issue_write_same() interface): > > Mike> ------------[ cut here ]------------ kernel BUG at > Mike> drivers/scsi/scsi_lib.c:1116! > > That's the > > BUG_ON(!req->nr_phys_segments); > > in scsi_setup_blk_pc_cmnd(). We set nr_phys_segments to bi_phys_segments > just before calling that function. So how did you end up with a > zero-segment bio? All I did was apply this patch to your writesame2 branch: http://people.redhat.com/msnitzer/patches/upstream/dm-io-WRITE_SAME/dm-thin-use-WRITE_SAME-for-zeroing.patch (and run the thinp-test-suite test I referenced in the other mail). -- I'm just using the blkdev_issue_write_same() interface.. nothing special I think the bio_has_data() change is at the heart of the BUG_ON(). Now this branch in blk_rq_bio_prep() is no longer taken: if (bio_has_data(bio)) { rq->nr_phys_segments = bio_phys_segments(q, bio); rq->buffer = bio_data(bio); } This patch fixed the issue for me (though I'm still missing why bio->bi_phys_segments was 0 given blkdev_issue_write_same() sets it): Index: linux-2.6/drivers/scsi/sd.c =================================================================== --- linux-2.6.orig/drivers/scsi/sd.c +++ linux-2.6/drivers/scsi/sd.c @@ -697,6 +697,7 @@ out: static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq) { struct scsi_disk *sdkp = scsi_disk(rq->rq_disk); + struct request_queue *q = sdkp->disk->queue; struct bio *bio = rq->bio; sector_t sector = bio->bi_sector; unsigned int nr_sectors = bio_sectors(bio); @@ -711,7 +712,8 @@ static int sd_setup_write_same_cmnd(stru rq->timeout = SD_WRITE_SAME_TIMEOUT; rq->__data_len = rq->resid_len = sdp->sector_size; - rq->nr_phys_segments = bio->bi_phys_segments; + rq->nr_phys_segments = bio_phys_segments(q, bio); + rq->buffer = bio_data(bio); memset(rq->cmd, 0, rq->cmd_len); if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff) { > PS. I was unsuccessful in getting the thinp test suite working. If you > can come up with a simpler way for me to get DM to issue a write same > then please share... Any new block that gets provisioned will trigger zeroing (unless the entire block will be consumed with data -- in that case the zero is avoided). If you use the default thinp block size: a random IO benchmark or a simple dd, of a partial block, should trigger zeroing. Mike -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
