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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs