>
> On Fri, Jan 02, 2015 at 06:40:54PM +0900, Namjae Jeon wrote:
> > 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 <namjae.jeon@xxxxxxxxxxx>
> > Signed-off-by: Ashish Sangwan <a.sangwan@xxxxxxxxxxx>
> > Cc: Brian Foster<bfoster@xxxxxxxxxx>
> > ---
>
> Hi Namjae,
Hi Brian,
>
> Just a few small things...
>
> > + } 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.
>
> > + */
> > + 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.
> > - /*
> > - * 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.
>
> > +
> > +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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
