xfs
[Top] [All Lists]

Re: [PATCH 3/3] block: implement (some of) fallocate for block devices

To: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>, <axboe@xxxxxxxxx>, <akpm@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] block: implement (some of) fallocate for block devices
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Date: Wed, 28 Sep 2016 18:42:14 -0700
Authentication-results: spf=pass (sender IP is 63.163.107.21) smtp.mailfrom=sandisk.com; infradead.org; dkim=none (message not signed) header.d=none;infradead.org; dmarc=bestguesspass action=none header.from=sandisk.com;
Cc: <linux-block@xxxxxxxxxxxxxxx>, <tytso@xxxxxxx>, <martin.petersen@xxxxxxxxxx>, <snitzer@xxxxxxxxxx>, <linux-api@xxxxxxxxxxxxxxx>, <bfoster@xxxxxxxxxx>, <xfs@xxxxxxxxxxx>, <hch@xxxxxxxxxxxxx>, <dm-devel@xxxxxxxxxx>, <linux-fsdevel@xxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sandiskcorp.onmicrosoft.com; s=selector1-sandisk-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=w9v1gGjGzPxlsqi3/l6i0PKiZ37aQDaV/vzrp+zhyO8=; b=IEWtHZC3gQOYTI86ph1ZTIeeVes7JB5MG34/sRxW5KuBRGmOyWJm9q3Fp1uTLUVJxO32coqXf4v0pl3ngkvkg6H72et6h6/cSGeFbDjHZEG5sFSJyKJMKYPYYLnEdz+d5GX1NeiCas8VJC2y7dgf37j3pyxt9DWXcXMI8MEP9QU=
In-reply-to: <147510959149.8940.2897845352082568677.stgit@xxxxxxxxxxxxxxxx>
References: <147510957066.8940.13803086684642725401.stgit@xxxxxxxxxxxxxxxx> <147510959149.8940.2897845352082568677.stgit@xxxxxxxxxxxxxxxx>
Spamdiagnosticmetadata: NSPM
Spamdiagnosticoutput: 1:99
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0
On 09/28/16 17:39, Darrick J. Wong wrote:
+       if (end > isize) {
+               if (mode & FALLOC_FL_KEEP_SIZE) {
+                       len = isize - start;
+                       end = start + len - 1;
+               } else
+                       return -EINVAL;
+       }

If FALLOC_FL_KEEP_SIZE has been set and end == isize the above code won't reduce end to isize - 1. Shouldn't "end > isize" be changed into "end >= isize" ?

+       switch (mode) {
+       case FALLOC_FL_ZERO_RANGE:
+       case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE:
+               error = blkdev_issue_zeroout(bdev, start >> 9, len >> 9,
+                                           GFP_KERNEL, false);
+               if (error)
+                       return error;
+               break;
+       case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE:
+               /* Only punch if the device can do zeroing discard. */
+               if (!blk_queue_discard(q) || !q->limits.discard_zeroes_data)
+                       return -EOPNOTSUPP;
+               error = blkdev_issue_discard(bdev, start >> 9, len >> 9,
+                                            GFP_KERNEL, 0);
+               if (error)
+                       return error;
+               break;
+       case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | 
FALLOC_FL_NO_HIDE_STALE:
+               error = blkdev_issue_discard(bdev, start >> 9, len >> 9,
+                                            GFP_KERNEL, 0);
+               if (error)
+                       return error;
+               break;
+       default:
+               return -EOPNOTSUPP;
+       }

Have you considered to move "if (error) return error" out of the switch statement?

+       /*
+        * Invalidate again; if someone wandered in and dirtied a page,
+        * the caller will be given -EBUSY;
+        */
+       return invalidate_inode_pages2_range(mapping,
+                                            start >> PAGE_SHIFT,
+                                            end >> PAGE_SHIFT);

A comment might be appropriate here that since end is inclusive and since the third argument of invalidate_inode_pages2_range() is inclusive that rounding down will yield the correct result.

Bart.

<Prev in Thread] Current Thread [Next in Thread>