RE: [PATCH v6 2/10] xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate
> >
> > >
> > > That said, I wonder whether we even care about a merge in the right
> > > shift case since we haven't punched a hole in the file and thus have not
> > > changed the "neighbors" of any of the extents we're shuffling around. I
> > > would think any extents that are already contiguous as such are already
> > > a single extent.
> > yes, in case of insert range it is highly unlikely that a merge is required.
> > But we have kept this code as part of a generic API for shifting extents.
> >
>
> I'm not opposed to that in principle, but the right shift is a separate
> invocation (at least in this incarnation) so it's of no consequence to
> the left shift if we were to drop it here. As far as I can tell, it's
> also broken in that if we ever were to hit it, it looks like it would
> perform a left-shift-merge in the middle of a broader right-shift
> sequence and probably corrupt the file and cause the insert range to
> fail.
>
> To fix it, I suspect we'd have to write a new helper to do the
> right-shift-merge appropriately and then probably want a way to test it.
> The only thing that comes to mind to accomplish that is to perhaps hook
> up the bmap split mechanism to an XFS ioctl() such that it could be
> invoked by xfs_io or some such tool. Unless I'm missing something,
> that's a bunch of extra work to handle a condition that probably should
> never occur.
>
> As it is, I'd suggest we drop it, add a small comment as to why there's
> no merge in that case, and perhaps consider an assert or warn_on_once
> type sanity check should we come across something unexpected in this
> codepath (like separate, but contiguous extents).
Okay, I agree, will drop it and add warn_on_once.
>
> > > > + }
> > > > +
> > > > + /*
> > > > +* Convert to a btree if necessary.
> > > > +*/
> > > > + if (xfs_bmap_needs_btree(ip, whichfork)) {
> > > > + int tmp_logflags; /* partial log flag return val */
> > > > +
> > > > + ASSERT(cur == NULL);
> > > > + error = xfs_bmap_extents_to_btree(tp, ip, firstfsb,
> > > > free_list,
> > > > + &cur, 0, &tmp_logflags, whichfork);
> > > > + logflags |= tmp_logflags;
> > > > + }
> > >
> > > Hmm, looks Ok, but it would be nice if we had a test case for this
> > > convert to btree scenario. I suspect something that falloc's just the
> > > right number extents for a known fs format and does an insert range
> > > right in the middle of one would suffice (and probably only require a
> > > few seconds to run).
> > Okay, I will prepare a testcase for convert to btree scenario of insert
> > range.
> > for collapse range we have generic/017 which tests multiple collapse
> > calls on same file. I can write same test for insert range which will
> > insert a single block hole at every alternate block in the file.
> > Each insert range call will split the extent into 2 extents. This test
> > need not be fs specfic so can be used for ext4 also.
> >
>
> That sounds like a nice idea. If you start with 1 or some sufficiently
> small number of extents, that should catch the inode format conversion
> induced by insert range at some point.
>
> It might also be interesting to consider following the insert range
> sequence with collapse range of all/some of the previously inserted
> ranges. That should give us some test coverage of the collapse extent
> merge code, if we're lacking that right now.
Good idea, I will do 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);
> > > > + return error;
> > > > +}
> > > > +
> > > > +int
> > > > +xfs_bmap_split_extent(
> > > > + struct xfs_inode*ip,
> > > > + xfs_fileoff_t split_fsb)
> > >
> > > You can line up the above params with the local vars below.
> > Okay.
> >
> > >
> > > > +{
> > > > + struct xfs_mount*mp = ip->i_mount;
> > > > + struct xfs_trans*tp;
> > > > + struct xfs_bmap_freefree_list;
> > > > + xfs_fsblock_t firstfsb;
> > > > + int committed;
> > > > + int error;
> > > > +
> > > > + tp = xfs_trans_alloc(mp, XFS_TRANS_DIOSTRAT);
> > > > + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
> > > > + XFS_DIOSTRAT_SPACE_RES(mp, 0), 0);
> > > > + if (error) {
> > > > + xfs_trans_cancel(tp, 0);
> > > > + return error;
> > > > + }
> > > > +
> > > > + xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > > + error = xfs_trans_reserve_quota(tp, mp, ip->i_udquot,
> > > > + ip->i_gdquot, ip->i_pdquot,
Re: [PATCH v6 2/10] xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate
On Thu, Dec 04, 2014 at 08:19:50PM +0900, Namjae Jeon wrote:
> >
> > Hi Namjae,
> Hi Brian,
>
> >
> > Here are some review notes. I haven't got to any of the test code or
> > played around with it just yet...
> Thanks for your reivew :)
>
No problem. :)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 79c9819..da01890 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -5528,56 +5528,87 @@ xfs_bmse_shift_one(
> >
> > FYI, you're probably going to need to rebase the xfs_bmse_shift_one()
> > bits on top of Dave's recent cleanup here:
> >
> > http://oss.sgi.com/archives/xfs/2014-11/msg00458.html
> okay.
>
> >
> > > int *current_ext,
> > > struct xfs_bmbt_rec_host*gotp,
> > > struct xfs_btree_cur*cur,
> > > - int *logflags)
> > > + int *logflags,
> > > + enum SHIFT_DIRECTIONSHIFT)
> > > {
> > > struct xfs_ifork*ifp;
> > > xfs_fileoff_t startoff;
> > > - struct xfs_bmbt_rec_host*leftp;
> > > + struct xfs_bmbt_rec_host*contp;
> > > struct xfs_bmbt_irecgot;
> > > - struct xfs_bmbt_irecleft;
> > > + struct xfs_bmbt_ireccont;
> > > 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_GOTO(!isnullstartblock(got.br_startblock),
> > > out_error);
> > >
> > > - /*
> > > - * If this is the first extent in the file, make sure there's enough
> > > - * room at the start of the file and jump right to the shift as there's
> > > - * no left extent to merge.
> > > - */
> > > - if (*current_ext == 0) {
> > > - if (got.br_startoff < offset_shift_fsb)
> > > + if (SHIFT == SHIFT_LEFT) {
> > > + startoff = got.br_startoff - offset_shift_fsb;
> > > + /*
> > > + * If this is the first extent in the file, make sure there's
> > > + * enough room at the start of the file and jump right to the
> > > + * shift as there's no left extent to merge.
> > > + */
> > > + if (*current_ext == 0) {
> > > + if (got.br_startoff < offset_shift_fsb)
> > > + return -EINVAL;
> > > + goto shift_extent;
> > > + }
> > > +
> > > + /* 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;
> > > - goto shift_extent;
> > > - }
> > >
> > > - /* 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);
> > > + /* check whether to merge the extent or shift it down */
> > > + if (!xfs_bmse_can_merge(&cont, &got, offset_shift_fsb))
> > > + goto shift_extent;
> > >
> > > - if (startoff < left.br_startoff + left.br_blockcount)
> > > - return -EINVAL;
> > > + return xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
> > > + *current_ext, gotp, contp, cur, logflags);
> > > + } else {
> > > + startoff = got.br_startoff + offset_shift_fsb;
> > > + /*
> > > + * If this is the last extent in the file, make sure there's
> > > + * enough room at the end of the file and jump right to the
> > > + * shift as there's no right extent to merge.
> > > + */
> > > + if (*current_ext == (total_extents - 1))
> > > + goto shift_extent;
> > >
> > > - /* check whether to merge the extent or shift it down */
> > > - if (!xfs_bmse_can_merge(&left, &got, offset_shift_fsb))
> > > - goto shift_extent;
> > > + /* grab the right extent and check for a large enough hole */
> > > + contp = xfs_iext_get_ext(ifp, *current_ext + 1);
> > > + xfs_bmbt_get_all(contp, &cont);
> > >
> > > - return xfs_bmse_merge(ip, whichfork, offset_shift_fsb, *current_ext,
> > > - gotp, leftp, cur, logflags);
> > > + if (startoff > cont.br_startoff)
> > > + return -EINVAL;
> >
> > Shouldn't this be 'if (startoff + got.br_blockount > cont.br_startoff)'?
> True, I will fix.
>
> >
> > > +
> > > + if (!xfs_bmse_can_merge(&got, &cont, offset_shift_fsb))
> > > + goto shift_extent;
> > > +
> >
RE: [PATCH v6 2/10] xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate
>
> Hi Namjae,
Hi Brian,
>
> Here are some review notes. I haven't got to any of the test code or
> played around with it just yet...
Thanks for your reivew :)
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 79c9819..da01890 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -5528,56 +5528,87 @@ xfs_bmse_shift_one(
>
> FYI, you're probably going to need to rebase the xfs_bmse_shift_one()
> bits on top of Dave's recent cleanup here:
>
> http://oss.sgi.com/archives/xfs/2014-11/msg00458.html
okay.
>
> > int *current_ext,
> > struct xfs_bmbt_rec_host*gotp,
> > struct xfs_btree_cur*cur,
> > - int *logflags)
> > + int *logflags,
> > + enum SHIFT_DIRECTIONSHIFT)
> > {
> > struct xfs_ifork*ifp;
> > xfs_fileoff_t startoff;
> > - struct xfs_bmbt_rec_host*leftp;
> > + struct xfs_bmbt_rec_host*contp;
> > struct xfs_bmbt_irecgot;
> > - struct xfs_bmbt_irecleft;
> > + struct xfs_bmbt_ireccont;
> > 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_GOTO(!isnullstartblock(got.br_startblock),
> > out_error);
> >
> > - /*
> > -* If this is the first extent in the file, make sure there's enough
> > -* room at the start of the file and jump right to the shift as there's
> > -* no left extent to merge.
> > -*/
> > - if (*current_ext == 0) {
> > - if (got.br_startoff < offset_shift_fsb)
> > + if (SHIFT == SHIFT_LEFT) {
> > + startoff = got.br_startoff - offset_shift_fsb;
> > + /*
> > +* If this is the first extent in the file, make sure there's
> > +* enough room at the start of the file and jump right to the
> > +* shift as there's no left extent to merge.
> > +*/
> > + if (*current_ext == 0) {
> > + if (got.br_startoff < offset_shift_fsb)
> > + return -EINVAL;
> > + goto shift_extent;
> > + }
> > +
> > + /* 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;
> > - goto shift_extent;
> > - }
> >
> > - /* 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);
> > + /* check whether to merge the extent or shift it down */
> > + if (!xfs_bmse_can_merge(&cont, &got, offset_shift_fsb))
> > + goto shift_extent;
> >
> > - if (startoff < left.br_startoff + left.br_blockcount)
> > - return -EINVAL;
> > + return xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
> > + *current_ext, gotp, contp, cur, logflags);
> > + } else {
> > + startoff = got.br_startoff + offset_shift_fsb;
> > + /*
> > +* If this is the last extent in the file, make sure there's
> > +* enough room at the end of the file and jump right to the
> > +* shift as there's no right extent to merge.
> > +*/
> > + if (*current_ext == (total_extents - 1))
> > + goto shift_extent;
> >
> > - /* check whether to merge the extent or shift it down */
> > - if (!xfs_bmse_can_merge(&left, &got, offset_shift_fsb))
> > - goto shift_extent;
> > + /* grab the right extent and check for a large enough hole */
> > + contp = xfs_iext_get_ext(ifp, *current_ext + 1);
> > + xfs_bmbt_get_all(contp, &cont);
> >
> > - return xfs_bmse_merge(ip, whichfork, offset_shift_fsb, *current_ext,
> > - gotp, leftp, cur, logflags);
> > + if (startoff > cont.br_startoff)
> > + return -EINVAL;
>
> Shouldn't this be 'if (startoff + got.br_blockount > cont.br_startoff)'?
True, I will fix.
>
> > +
> > + if (!xfs_bmse_can_merge(&got, &cont, offset_shift_fsb))
> > + goto shift_extent;
> > +
> > + return xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
> > + *current_ext + 1, contp, gotp, cur,
> > +
Re: [PATCH v6 2/10] xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate
On Mon, Nov 24, 2014 at 03:16:33PM +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
> Signed-off-by: Ashish Sangwan
> ---
Hi Namjae,
Here are some review notes. I haven't got to any of the test code or
played around with it just yet...
> Changelog
>
> v6:
> - This version is based upon Brian's changes to collapse paths.
> - Instead of having seperate functions for shifting extents left/right, the
>current extent shift function is made generic to shift in both directions.
>
> v5:
> - remove allocation part.
>
> v4:
> - set cur->bc_private.b.allocated to zero before calling
> xfs_btree_del_cursor.
>
> v3:
> - remove XFS_TRANS_RESERVE and assert.
> - update the comment of blockcount calculation.
> - use 'if(blockcount)' instead of 'if (got.br_blockcount < blockcount)'.
> - move insert_file_space() calling under xfs_setattr_size to avoid code
> duplicate.
>
> v2:
> - remove reserved enable.
> - add xfs_qm_dqattach.
> - reset blockcount in xfs_bmap_shift_extents_right.
> - update i_size to avoid data loss before insert_file_space() is called.
> - use in-memory extent array size that delayed allocation extents
>
> fs/xfs/libxfs/xfs_bmap.c | 368
> +--
> fs/xfs/libxfs/xfs_bmap.h | 14 +-
> fs/xfs/xfs_bmap_util.c | 118 +++
> fs/xfs/xfs_bmap_util.h | 2 +
> fs/xfs/xfs_file.c| 38 -
> fs/xfs/xfs_trace.h | 1 +
> 6 files changed, 463 insertions(+), 78 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 79c9819..da01890 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5528,56 +5528,87 @@ xfs_bmse_shift_one(
FYI, you're probably going to need to rebase the xfs_bmse_shift_one()
bits on top of Dave's recent cleanup here:
http://oss.sgi.com/archives/xfs/2014-11/msg00458.html
> int *current_ext,
> struct xfs_bmbt_rec_host*gotp,
> struct xfs_btree_cur*cur,
> - int *logflags)
> + int *logflags,
> + enum SHIFT_DIRECTIONSHIFT)
> {
> struct xfs_ifork*ifp;
> xfs_fileoff_t startoff;
> - struct xfs_bmbt_rec_host*leftp;
> + struct xfs_bmbt_rec_host*contp;
> struct xfs_bmbt_irecgot;
> - struct xfs_bmbt_irecleft;
> + struct xfs_bmbt_ireccont;
> 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_GOTO(!isnullstartblock(got.br_startblock),
> out_error);
>
> - /*
> - * If this is the first extent in the file, make sure there's enough
> - * room at the start of the file and jump right to the shift as there's
> - * no left extent to merge.
> - */
> - if (*current_ext == 0) {
> - if (got.br_startoff < offset_shift_fsb)
> + if (SHIFT == SHIFT_LEFT) {
> + startoff = got.br_startoff - offset_shift_fsb;
> + /*
> + * If this is the first extent in the file, make sure there's
> + * enough room at the start of the file and jump right to the
> + * shift as there's no left extent to merge.
> + */
> + if (*current_ext == 0) {
> + if (got.br_startoff < offset_shift_fsb)
> + return -EINVAL;
> + goto shift_extent;
> + }
> +
> + /* 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;
> - goto shift_extent;
> - }
>
> - /* grab the left extent and check for a large enough hole */
> - leftp = xfs_iext_get_ext(ifp, *current_ext - 1);
> - xfs_
[PATCH v6 2/10] xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate
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
Signed-off-by: Ashish Sangwan
---
Changelog
v6:
- This version is based upon Brian's changes to collapse paths.
- Instead of having seperate functions for shifting extents left/right, the
current extent shift function is made generic to shift in both directions.
v5:
- remove allocation part.
v4:
- set cur->bc_private.b.allocated to zero before calling xfs_btree_del_cursor.
v3:
- remove XFS_TRANS_RESERVE and assert.
- update the comment of blockcount calculation.
- use 'if(blockcount)' instead of 'if (got.br_blockcount < blockcount)'.
- move insert_file_space() calling under xfs_setattr_size to avoid code
duplicate.
v2:
- remove reserved enable.
- add xfs_qm_dqattach.
- reset blockcount in xfs_bmap_shift_extents_right.
- update i_size to avoid data loss before insert_file_space() is called.
- use in-memory extent array size that delayed allocation extents
fs/xfs/libxfs/xfs_bmap.c | 368 +--
fs/xfs/libxfs/xfs_bmap.h | 14 +-
fs/xfs/xfs_bmap_util.c | 118 +++
fs/xfs/xfs_bmap_util.h | 2 +
fs/xfs/xfs_file.c| 38 -
fs/xfs/xfs_trace.h | 1 +
6 files changed, 463 insertions(+), 78 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 79c9819..da01890 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5528,56 +5528,87 @@ xfs_bmse_shift_one(
int *current_ext,
struct xfs_bmbt_rec_host*gotp,
struct xfs_btree_cur*cur,
- int *logflags)
+ int *logflags,
+ enum SHIFT_DIRECTIONSHIFT)
{
struct xfs_ifork*ifp;
xfs_fileoff_t startoff;
- struct xfs_bmbt_rec_host*leftp;
+ struct xfs_bmbt_rec_host*contp;
struct xfs_bmbt_irecgot;
- struct xfs_bmbt_irecleft;
+ struct xfs_bmbt_ireccont;
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_GOTO(!isnullstartblock(got.br_startblock),
out_error);
- /*
-* If this is the first extent in the file, make sure there's enough
-* room at the start of the file and jump right to the shift as there's
-* no left extent to merge.
-*/
- if (*current_ext == 0) {
- if (got.br_startoff < offset_shift_fsb)
+ if (SHIFT == SHIFT_LEFT) {
+ startoff = got.br_startoff - offset_shift_fsb;
+ /*
+* If this is the first extent in the file, make sure there's
+* enough room at the start of the file and jump right to the
+* shift as there's no left extent to merge.
+*/
+ if (*current_ext == 0) {
+ if (got.br_startoff < offset_shift_fsb)
+ return -EINVAL;
+ goto shift_extent;
+ }
+
+ /* 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;
- goto shift_extent;
- }
- /* 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);
+ /* check whether to merge the extent or shift it down */
+ if (!xfs_bmse_can_merge(&cont, &got, offset_shift_fsb))
+ goto shift_extent;
- if (startoff < left.br_startoff + left.br_blockcount)
- return -EINVAL;
+ return xfs_bmse_merge(ip, whichfork, offset_shift_fsb,
+ *current_ext, gotp, contp, cur, logflags);
+ } else {
