Re: [PATCH 0/2] MMC discard support (was [PATCH 0/7] Discard requests, v2)

[Jens Axboe <jens.axboe@oracle.com>]

On Sat, Aug 16 2008, Pierre Ossman wrote:
> I've cooked up some patches that maps David's new discard requests to
> erase operations on MMC and SD cards. I'm not entirely sure these are
> something to keep though as I've been unable to see any performance
> increase in keeping blocks erased. Do we have any other reason to keep
> it?
> 
> During development and test I noticed one issue though; the discard
> requests are chopped up into smaller pieces. As the erase commands have
> a granularity, sometimes this can mean that parts of the flash are
> missed because the discard request isn't split in alignment with the
> erase boundaries.
> 
> For this reason, I propose that discard requests do not respect the
> regular queue restrictions. Those are generally for expressing
> limitations in the devices' DMA engines. Since the discard request
> carries no data, the DMA engine will never get involved.

I agree with that, the thought did cross my mind earlier as well. I
committed something like the below (in two patches). Do you want me to
queue up your mmc patches as well?

diff --git a/block/blk-core.c b/block/blk-core.c
index d785466..52824c0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1381,7 +1381,8 @@ end_io:
 			break;
 		}
 
-		if (unlikely(nr_sectors > q->max_hw_sectors)) {
+		if (unlikely(bio_has_data(bio) &&
+		     nr_sectors > q->max_hw_sectors)) {
 			printk(KERN_ERR "bio too big device %s (%u > %u)\n",
 				bdevname(bio->bi_bdev, b),
 				bio_sectors(bio),
diff --git a/block/ioctl.c b/block/ioctl.c
index 375c579..ec6fa5d 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -125,52 +125,41 @@ static int blk_ioctl_discard(struct block_device *bdev, uint64_t start,
 			     uint64_t len)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
+	DECLARE_COMPLETION_ONSTACK(wait);
+	struct bio *bio;
 	int ret = 0;
 
 	if (start & 511)
 		return -EINVAL;
 	if (len & 511)
 		return -EINVAL;
-	start >>= 9;
-	len >>= 9;
 
-	if (start + len > (bdev->bd_inode->i_size >> 9))
+	if (start + len > bdev->bd_inode->i_size)
 		return -EINVAL;
 
 	if (!q->prepare_discard_fn)
 		return -EOPNOTSUPP;
 
-	while (len && !ret) {
-		DECLARE_COMPLETION_ONSTACK(wait);
-		struct bio *bio;
-
-		bio = bio_alloc(GFP_KERNEL, 0);
-		if (!bio)
-			return -ENOMEM;
-
-		bio->bi_end_io = blk_ioc_discard_endio;
-		bio->bi_bdev = bdev;
-		bio->bi_private = &wait;
-		bio->bi_sector = start;
-
-		if (len > q->max_hw_sectors) {
-			bio->bi_size = q->max_hw_sectors << 9;
-			len -= q->max_hw_sectors;
-			start += q->max_hw_sectors;
-		} else {
-			bio->bi_size = len << 9;
-			len = 0;
-		}
-		submit_bio(DISCARD_NOBARRIER, bio);
-
-		wait_for_completion(&wait);
-
-		if (bio_flagged(bio, BIO_EOPNOTSUPP))
-			ret = -EOPNOTSUPP;
-		else if (!bio_flagged(bio, BIO_UPTODATE))
-			ret = -EIO;
-		bio_put(bio);
-	}
+	bio = bio_alloc(GFP_KERNEL, 0);
+	if (!bio)
+		return -ENOMEM;
+
+	bio->bi_end_io = blk_ioc_discard_endio;
+	bio->bi_bdev = bdev;
+	bio->bi_private = &wait;
+	bio->bi_sector = start >> 9;
+	bio->bi_size = len;
+
+	submit_bio(DISCARD_NOBARRIER, bio);
+
+	wait_for_completion(&wait);
+
+	if (bio_flagged(bio, BIO_EOPNOTSUPP))
+		ret = -EOPNOTSUPP;
+	else if (!bio_flagged(bio, BIO_UPTODATE))
+		ret = -EIO;
+
+	bio_put(bio);
 	return ret;
 }
 

-- 
Jens Axboe