On Wed, Sep 28, 2016 at 06:42:14PM -0700, Bart Van Assche wrote:
> 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" ?
Oops. Will fix and send out a v2.
> >+ 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?
Sure, I could do that.
> >+ /*
> >+ * 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.
/methot the documentation of invalidate_inode_pages2_range was clear
enough on that point, but I could throw that into the comment too.
--D
>
> Bart.
>
|