On Tue, Feb 17, 2015 at 12:47:49AM +0900, Namjae Jeon wrote:

Please don't shout. ;)

Lower case for types and variables, upper case for the enum values.
I also think the "shift" variable should be named "direction",
too, so the code reads "if (direction == SHIFT_LEFT)" and so is
clearly self documenting...

(only commenting once on this, please change it in other places)
as well ;)

 {
 	struct xfs_ifork		*ifp;
 	xfs_fileoff_t			startoff;
-	struct xfs_bmbt_rec_host	*leftp;
+	struct xfs_bmbt_rec_host	*contp;
 	struct xfs_bmbt_irec		got;
-	struct xfs_bmbt_irec		left;
+	struct xfs_bmbt_irec		cont;

Not sure what "cont" is short for. It's used as the "adjacent
extent" record, so that would be a better name IMO.

 	int				error;
 	int				i;
+	int				total_extents;
 
 	ifp = XFS_IFORK_PTR(ip, whichfork);
+	total_extents = ifp->if_bytes / sizeof(xfs_bmbt_rec_t);
 
 	xfs_bmbt_get_all(gotp, &got);
-	startoff = got.br_startoff - offset_shift_fsb;
 
 	/* delalloc extents should be prevented by caller */
 	XFS_WANT_CORRUPTED_RETURN(!isnullstartblock(got.br_startblock));
 
-	/*
-	 * Check for merge if we've got an extent to the left, otherwise make
-	 * sure there's enough room at the start of the file for the shift.
-	 */
-	if (*current_ext) {
-		/* grab the left extent and check for a large enough hole */
-		leftp = xfs_iext_get_ext(ifp, *current_ext - 1);
-		xfs_bmbt_get_all(leftp, &left);
+	if (SHIFT == SHIFT_LEFT) {
+		startoff = got.br_startoff - offset_shift_fsb;
 
-		if (startoff < left.br_startoff + left.br_blockcount)
+		/*
+		 * Check for merge if we've got an extent to the left,
+		 * otherwise make sure there's enough room at the start
+		 * of the file for the shift.
+		 */
+		if (*current_ext) {
+			/*
+			 * grab the left extent and check for a large
+			 * enough hole.
+			 */
+			contp = xfs_iext_get_ext(ifp, *current_ext - 1);
+			xfs_bmbt_get_all(contp, &cont);
+
+			if (startoff < cont.br_startoff + cont.br_blockcount)
+				return -EINVAL;
+
+			/* check whether to merge the extent or shift it down */
+			if (xfs_bmse_can_merge(&cont, &got, offset_shift_fsb)) {
+				return xfs_bmse_merge(ip, whichfork,
+						      offset_shift_fsb,
+						      *current_ext, gotp, contp,
+						      cur, logflags);
+			}
+		} else if (got.br_startoff < offset_shift_fsb)
 			return -EINVAL;

This would be better written:

		if (!*current_ext) {
			if (got.br_startoff < offset_shift_fsb)
				return -EINVAL;
			goto update_current_ext;
		}

and then the rest of the code in the shift left branch can drop a
level of indent and hence become less congested and easier to read.

+	} 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
+		 * accommodating the shift.
+		 */
+		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;
 
-		/* check whether to merge the extent or shift it down */
-		if (xfs_bmse_can_merge(&left, &got, offset_shift_fsb)) {
-			return xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
-					      *current_ext, gotp, leftp, cur,
-					      logflags);
+			/*
+			 * 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.
+			 */
+			if (xfs_bmse_can_merge(&got, &cont,  offset_shift_fsb))
+				WARN_ON_ONCE(1);
 		}

Similarly:
		/* nothing to move if this is the last extent */
		if (*current_ext >= total_extents)
			goto update_current_ext;

-	} else if (got.br_startoff < offset_shift_fsb)
-		return -EINVAL;
-
+	}
 	/*
 	 * Increment the extent index for the next iteration, update the start
 	 * offset of the in-core extent and update the btree if applicable.
 	 */
-	(*current_ext)++;

update_current_ext:

This should be at the top of the function. i.e.

	ASSERT(*next_fsb != NULLFSBLOCK || direction == SHIFT_RIGHT)

+		gotp = xfs_iext_get_ext(ifp, total_extents - 1);
+		xfs_bmbt_get_all(gotp, &got);
+		*next_fsb = got.br_startoff;
+		if (stop_fsb > *next_fsb) {
+			*done = 1;
+			goto del_cursor;
+		}
+	}
+
+	/* Lookup the extent index at which we have to stop */
+	if (SHIFT == SHIFT_RIGHT) {
+		gotp = xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
+		/* Make stop_extent exclusive of shift range */
+		stop_extent--;
+	} else
+		stop_extent = total_extents;
+
+	/*
 	 * Look up the extent index for the fsb where we start shifting. We can
 	 * henceforth iterate with current_ext as extent list changes are locked
 	 * out via ilock.
 	 *
 	 * gotp can be null in 2 cases: 1) if there are no extents or 2)
-	 * start_fsb lies in a hole beyond which there are no extents. Either
+	 * *next_fsb lies in a hole beyond which there are no extents. Either
 	 * way, we are done.
 	 */
-	gotp = xfs_iext_bno_to_ext(ifp, start_fsb, &current_ext);
+	gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, &current_ext);
 	if (!gotp) {
 		*done = 1;
 		goto del_cursor;
 	}
 
-	/*
-	 * 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;

		error = -EIO;

+		goto del_cursor;
+	}
+
+	while (nexts++ < num_exts) {
 		error = xfs_bmse_shift_one(ip, whichfork, offset_shift_fsb,
-					&current_ext, gotp, cur, &logflags);
+					   &current_ext, gotp, cur, &logflags,
+					   SHIFT);
 		if (error)
 			goto del_cursor;
+		/*
+		 * In case there was an extent merge after shifting extent,
+		 * extent numbers would change.
+		 * Update total extent count and grab the next record.
+		 */

		/*
		 * If there was an extent merge during the shift, the extent
		 * count can change. Update the total and grade the next record.
		 */

Comment can go before the call to xfs_iext_bno_to_ext().

+
+	xfs_bmbt_get_all(gotp, &got);
+
+	/*
+	 * Check split_fsb lies in a hole or the start boundary offset
+	 * of the extent.
+	 */
+	if (got.br_startoff >= split_fsb)
+		return 0;
+
+	gotblkcnt = split_fsb - got.br_startoff;
+	new.br_startoff = split_fsb;
+	new.br_startblock = got.br_startblock + gotblkcnt;
+	new.br_blockcount = got.br_blockcount - gotblkcnt;
+	new.br_state = got.br_state;
+
+	if (ifp->if_flags & XFS_IFBROOT) {
+		cur = xfs_bmbt_init_cursor(mp, tp, ip, whichfork);
+		cur->bc_private.b.firstblock = *firstfsb;
+		cur->bc_private.b.flist = free_list;
+		cur->bc_private.b.flags = 0;
+	}
+
+	if (cur) {

No need to close the XFS_IFBROOT branch and then check for cur;
we just allocated it inside the XFS_IFBROOT branch!

+		error = xfs_bmbt_lookup_eq(cur, got.br_startoff,
+				got.br_startblock,
+				got.br_blockcount,
+				&i);
+		if (error)
+			goto del_cursor;
+		XFS_WANT_CORRUPTED_GOTO(i == 1, del_cursor);
+	}

....

This needs a comment explaining why we are splitting an extent here.

bool rather than int.

 
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |
-		     FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE))
+		     FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |
+		     FALLOC_FL_INSERT_RANGE))
 		return -EOPNOTSUPP;

This should use a local define before the function such as:

#define XFS_FALLOC_FL_SUPPORTED		\
		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | \
		 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE | \
		 FALLOC_FL_INSERT_RANGE)

This is similar to how we define supported checks for FIEMAP
operations in xfs_vn_fiemap().

At first I thought that was a duplicate check of what is in
vfs_fallocate() (i.e. off + len > s_maxbytes). Can you change the
comment to read something like:

		/* check the new inode size does not wrap through zero */

+
+		/* Offset should be less than i_size */
+		if (offset >= i_size_read(inode)) {
+			error = -EINVAL;
+			goto out_unlock;
+		}
+
+		new_size = i_size_read(inode) + len;
+		do_file_insert = 1;

Why do you use inode->i_size onthe wrap check, yet i_size_read()
twice here?

	/*
	 * Perform hole insertion now that the file size has been
	 * updated so that if we crash during the operation we don't
	 * leave shifted extents past EOF and hence losing access to
	 * the data that is contained within them.
	 */

+	if (do_file_insert)
+		error = xfs_insert_file_space(ip, offset, len);
+
 out_unlock:
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
 	return error;

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html