On Fri, Jan 02, 2015 at 06:40:54PM +0900, Namjae Jeon wrote:

quoted

This patch implements fallocate's FALLOC_FL_INSERT_RANGE for XFS.

1) Make sure that both offset and len are block size aligned.
2) Update the i_size of inode by len bytes.
3) Compute the file's logical block number against offset. If the computed
   block number is not the starting block of the extent, split the extent
   such that the block number is the starting block of the extent.
4) Shift all the extents which are lying bewteen [offset, last allocated extent]
   towards right by len bytes. This step will make a hole of len bytes
   at offset.

Signed-off-by: Namjae Jeon <redacted>
Signed-off-by: Ashish Sangwan <redacted>
Cc: Brian Foster<redacted>
---

Hi Namjae,

Hi Brian,

Just a few small things...

quoted

+	} else {
+		startoff = got.br_startoff + offset_shift_fsb;
+		/*
+		 * If this is not the last extent in the file, make sure there's
+		 * enough room between current extent and next extent for
+		 * accomodating the shift.

Spelling nit:	   accommodating

Okay, I will fix it.

quoted

+		 */
+		if (*current_ext < (total_extents - 1)) {
+			contp = xfs_iext_get_ext(ifp, *current_ext + 1);
+			xfs_bmbt_get_all(contp, &cont);
+			if (startoff + got.br_blockcount > cont.br_startoff)
+				return -EINVAL;
+			if (xfs_bmse_can_merge(&got, &cont,  offset_shift_fsb))
+				WARN_ON_ONCE(1);

Ok, but a comment before the check would be nice should somebody have to
look up the warning that fires here. E.g.,:

/*
 * Unlike a left shift (which involves a hole punch), a right shift does
 * not modify extent neighbors in any way. We should never find
 * mergeable extents in this scenario. Check anyways and warn if we
 * encounter two extents that could be one.
 */

Okay, I will update it.

quoted

-	/*
-	 * There may be delalloc extents in the data fork before the range we
-	 * are collapsing out, so we cannot use the count of real extents here.
-	 * Instead we have to calculate it from the incore fork.
-	 */
-	total_extents = ifp->if_bytes / sizeof(xfs_bmbt_rec_t);
-	while (nexts++ < num_exts && current_ext < total_extents) {
+	/* some sanity checking before we finally start shifting extents */
+	if ((SHIFT == SHIFT_LEFT && current_ext > stop_extent) ||
+	     (SHIFT == SHIFT_RIGHT && current_ext < stop_extent)) {
+		error = EIO;
+		goto del_cursor;
+	}

If stop_extent is consistently exclusive now, we probably need to use >=
and <= here (e.g., 'stop_extent' should never be shifted).

You're Right. I will fix it.

quoted

+

quoted

+del_cursor:
+	if (cur) {
+		cur->bc_private.b.allocated = 0;
+		xfs_btree_del_cursor(cur,
+				error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
+	}
+	xfs_trans_log_inode(tp, ip, logflags);

	if (logflags)
		xfs_trans_log_inode(tp, ip, logflags);

Okay.

Otherwise, the rest looks pretty good. I'll try to do some testing with
it soon.

Thanks very much for your review!!

Brian

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