| Message ID | 20200721001606.10781-11-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Mon, Jul 20, 2020 at 05:15:51PM -0700, Allison Collins wrote: > Refactor xfs_attr_rmtval_remove to add helper function > __xfs_attr_rmtval_remove. We will use this later when we introduce > delayed attributes. This function will eventually replace > xfs_attr_rmtval_remove > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++--------- > fs/xfs/libxfs/xfs_attr_remote.h | 1 + > 2 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c > index 4d51969..9b4c173 100644 > --- a/fs/xfs/libxfs/xfs_attr_remote.c > +++ b/fs/xfs/libxfs/xfs_attr_remote.c > @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove( > xfs_dablk_t lblkno; > int blkcnt; > int error = 0; > - int done = 0; > + int retval = 0; > > trace_xfs_attr_rmtval_remove(args); > > @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove( > */ > lblkno = args->rmtblkno; > blkcnt = args->rmtblkcnt; Er... I think these local variables can go away here, right? --D > - while (!done) { > - error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt, > - XFS_BMAPI_ATTRFORK, 1, &done); > - if (error) > - return error; > - error = xfs_defer_finish(&args->trans); > - if (error) > - return error; > + do { > + retval = __xfs_attr_rmtval_remove(args); > + if (retval && retval != EAGAIN) > + return retval; > > /* > * Close out trans and start the next one in the chain. > @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove( > error = xfs_trans_roll_inode(&args->trans, args->dp); > if (error) > return error; > - } > + } while (retval == -EAGAIN); > + > return 0; > } > + > +/* > + * Remove the value associated with an attribute by deleting the out-of-line > + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the > + * transaction and re-call the function > + */ > +int > +__xfs_attr_rmtval_remove( > + struct xfs_da_args *args) > +{ > + int error, done; > + > + /* > + * Unmap value blocks for this attr. > + */ > + error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno, > + args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done); > + if (error) > + return error; > + > + error = xfs_defer_finish(&args->trans); > + if (error) > + return error; > + > + if (!done) > + return -EAGAIN; > + > + return error; > +} > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h > index 3616e88..9eee615 100644 > --- a/fs/xfs/libxfs/xfs_attr_remote.h > +++ b/fs/xfs/libxfs/xfs_attr_remote.h > @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args); > int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map, > xfs_buf_flags_t incore_flags); > int xfs_attr_rmtval_invalidate(struct xfs_da_args *args); > +int __xfs_attr_rmtval_remove(struct xfs_da_args *args); > #endif /* __XFS_ATTR_REMOTE_H__ */ > -- > 2.7.4 >
On Tue, Jul 21, 2020 at 04:31:18PM -0700, Darrick J. Wong wrote: > On Mon, Jul 20, 2020 at 05:15:51PM -0700, Allison Collins wrote: > > Refactor xfs_attr_rmtval_remove to add helper function > > __xfs_attr_rmtval_remove. We will use this later when we introduce > > delayed attributes. This function will eventually replace > > xfs_attr_rmtval_remove > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > > Reviewed-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++--------- > > fs/xfs/libxfs/xfs_attr_remote.h | 1 + > > 2 files changed, 37 insertions(+), 10 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c > > index 4d51969..9b4c173 100644 > > --- a/fs/xfs/libxfs/xfs_attr_remote.c > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c > > @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove( > > xfs_dablk_t lblkno; > > int blkcnt; > > int error = 0; > > - int done = 0; > > + int retval = 0; > > > > trace_xfs_attr_rmtval_remove(args); > > > > @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove( > > */ > > lblkno = args->rmtblkno; > > blkcnt = args->rmtblkcnt; > > Er... I think these local variables can go away here, right? > > --D > > > - while (!done) { > > - error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt, > > - XFS_BMAPI_ATTRFORK, 1, &done); > > - if (error) > > - return error; > > - error = xfs_defer_finish(&args->trans); > > - if (error) > > - return error; > > + do { > > + retval = __xfs_attr_rmtval_remove(args); > > + if (retval && retval != EAGAIN) Also this has to be -EAGAIN. Amazingly, nothing in fstests blew up on this. --D > > + return retval; > > > > /* > > * Close out trans and start the next one in the chain. > > @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove( > > error = xfs_trans_roll_inode(&args->trans, args->dp); > > if (error) > > return error; > > - } > > + } while (retval == -EAGAIN); > > + > > return 0; > > } > > + > > +/* > > + * Remove the value associated with an attribute by deleting the out-of-line > > + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the > > + * transaction and re-call the function > > + */ > > +int > > +__xfs_attr_rmtval_remove( > > + struct xfs_da_args *args) > > +{ > > + int error, done; > > + > > + /* > > + * Unmap value blocks for this attr. > > + */ > > + error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno, > > + args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done); > > + if (error) > > + return error; > > + > > + error = xfs_defer_finish(&args->trans); > > + if (error) > > + return error; > > + > > + if (!done) > > + return -EAGAIN; > > + > > + return error; > > +} > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h > > index 3616e88..9eee615 100644 > > --- a/fs/xfs/libxfs/xfs_attr_remote.h > > +++ b/fs/xfs/libxfs/xfs_attr_remote.h > > @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args); > > int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map, > > xfs_buf_flags_t incore_flags); > > int xfs_attr_rmtval_invalidate(struct xfs_da_args *args); > > +int __xfs_attr_rmtval_remove(struct xfs_da_args *args); > > #endif /* __XFS_ATTR_REMOTE_H__ */ > > -- > > 2.7.4 > >
On 7/21/20 4:31 PM, Darrick J. Wong wrote: > On Mon, Jul 20, 2020 at 05:15:51PM -0700, Allison Collins wrote: >> Refactor xfs_attr_rmtval_remove to add helper function >> __xfs_attr_rmtval_remove. We will use this later when we introduce >> delayed attributes. This function will eventually replace >> xfs_attr_rmtval_remove >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> >> Reviewed-by: Brian Foster <bfoster@redhat.com> >> --- >> fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++--------- >> fs/xfs/libxfs/xfs_attr_remote.h | 1 + >> 2 files changed, 37 insertions(+), 10 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c >> index 4d51969..9b4c173 100644 >> --- a/fs/xfs/libxfs/xfs_attr_remote.c >> +++ b/fs/xfs/libxfs/xfs_attr_remote.c >> @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove( >> xfs_dablk_t lblkno; >> int blkcnt; >> int error = 0; >> - int done = 0; >> + int retval = 0; >> >> trace_xfs_attr_rmtval_remove(args); >> >> @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove( >> */ >> lblkno = args->rmtblkno; >> blkcnt = args->rmtblkcnt; > > Er... I think these local variables can go away here, right? I think so, this whole function sort of get phased out over the last three patches, so they dont live long. Should be fine to clean out here though. Allison > > --D > >> - while (!done) { >> - error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt, >> - XFS_BMAPI_ATTRFORK, 1, &done); >> - if (error) >> - return error; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - return error; >> + do { >> + retval = __xfs_attr_rmtval_remove(args); >> + if (retval && retval != EAGAIN) >> + return retval; >> >> /* >> * Close out trans and start the next one in the chain. >> @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove( >> error = xfs_trans_roll_inode(&args->trans, args->dp); >> if (error) >> return error; >> - } >> + } while (retval == -EAGAIN); >> + >> return 0; >> } >> + >> +/* >> + * Remove the value associated with an attribute by deleting the out-of-line >> + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the >> + * transaction and re-call the function >> + */ >> +int >> +__xfs_attr_rmtval_remove( >> + struct xfs_da_args *args) >> +{ >> + int error, done; >> + >> + /* >> + * Unmap value blocks for this attr. >> + */ >> + error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno, >> + args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done); >> + if (error) >> + return error; >> + >> + error = xfs_defer_finish(&args->trans); >> + if (error) >> + return error; >> + >> + if (!done) >> + return -EAGAIN; >> + >> + return error; >> +} >> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h >> index 3616e88..9eee615 100644 >> --- a/fs/xfs/libxfs/xfs_attr_remote.h >> +++ b/fs/xfs/libxfs/xfs_attr_remote.h >> @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args); >> int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map, >> xfs_buf_flags_t incore_flags); >> int xfs_attr_rmtval_invalidate(struct xfs_da_args *args); >> +int __xfs_attr_rmtval_remove(struct xfs_da_args *args); >> #endif /* __XFS_ATTR_REMOTE_H__ */ >> -- >> 2.7.4 >>
On 7/21/20 5:24 PM, Darrick J. Wong wrote: > On Tue, Jul 21, 2020 at 04:31:18PM -0700, Darrick J. Wong wrote: >> On Mon, Jul 20, 2020 at 05:15:51PM -0700, Allison Collins wrote: >>> Refactor xfs_attr_rmtval_remove to add helper function >>> __xfs_attr_rmtval_remove. We will use this later when we introduce >>> delayed attributes. This function will eventually replace >>> xfs_attr_rmtval_remove >>> >>> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >>> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> >>> Reviewed-by: Brian Foster <bfoster@redhat.com> >>> --- >>> fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++--------- >>> fs/xfs/libxfs/xfs_attr_remote.h | 1 + >>> 2 files changed, 37 insertions(+), 10 deletions(-) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c >>> index 4d51969..9b4c173 100644 >>> --- a/fs/xfs/libxfs/xfs_attr_remote.c >>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c >>> @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove( >>> xfs_dablk_t lblkno; >>> int blkcnt; >>> int error = 0; >>> - int done = 0; >>> + int retval = 0; >>> >>> trace_xfs_attr_rmtval_remove(args); >>> >>> @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove( >>> */ >>> lblkno = args->rmtblkno; >>> blkcnt = args->rmtblkcnt; >> >> Er... I think these local variables can go away here, right? >> >> --D >> >>> - while (!done) { >>> - error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt, >>> - XFS_BMAPI_ATTRFORK, 1, &done); >>> - if (error) >>> - return error; >>> - error = xfs_defer_finish(&args->trans); >>> - if (error) >>> - return error; >>> + do { >>> + retval = __xfs_attr_rmtval_remove(args); >>> + if (retval && retval != EAGAIN) > > Also this has to be -EAGAIN. Amazingly, nothing in fstests blew up on > this. Ok, will fix! If you are running with the full set, it wouldnt trip over anything because this whole function is removed by the end of the series. Allison > > --D > >>> + return retval; >>> >>> /* >>> * Close out trans and start the next one in the chain. >>> @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove( >>> error = xfs_trans_roll_inode(&args->trans, args->dp); >>> if (error) >>> return error; >>> - } >>> + } while (retval == -EAGAIN); >>> + >>> return 0; >>> } >>> + >>> +/* >>> + * Remove the value associated with an attribute by deleting the out-of-line >>> + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the >>> + * transaction and re-call the function >>> + */ >>> +int >>> +__xfs_attr_rmtval_remove( >>> + struct xfs_da_args *args) >>> +{ >>> + int error, done; >>> + >>> + /* >>> + * Unmap value blocks for this attr. >>> + */ >>> + error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno, >>> + args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done); >>> + if (error) >>> + return error; >>> + >>> + error = xfs_defer_finish(&args->trans); >>> + if (error) >>> + return error; >>> + >>> + if (!done) >>> + return -EAGAIN; >>> + >>> + return error; >>> +} >>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h >>> index 3616e88..9eee615 100644 >>> --- a/fs/xfs/libxfs/xfs_attr_remote.h >>> +++ b/fs/xfs/libxfs/xfs_attr_remote.h >>> @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args); >>> int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map, >>> xfs_buf_flags_t incore_flags); >>> int xfs_attr_rmtval_invalidate(struct xfs_da_args *args); >>> +int __xfs_attr_rmtval_remove(struct xfs_da_args *args); >>> #endif /* __XFS_ATTR_REMOTE_H__ */ >>> -- >>> 2.7.4 >>>
On Fri, Jul 24, 2020 at 05:08:13PM -0700, Allison Collins wrote: > > > On 7/21/20 5:24 PM, Darrick J. Wong wrote: > > On Tue, Jul 21, 2020 at 04:31:18PM -0700, Darrick J. Wong wrote: > > > On Mon, Jul 20, 2020 at 05:15:51PM -0700, Allison Collins wrote: > > > > Refactor xfs_attr_rmtval_remove to add helper function > > > > __xfs_attr_rmtval_remove. We will use this later when we introduce > > > > delayed attributes. This function will eventually replace > > > > xfs_attr_rmtval_remove > > > > > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > > > > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > > > > Reviewed-by: Brian Foster <bfoster@redhat.com> > > > > --- > > > > fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++--------- > > > > fs/xfs/libxfs/xfs_attr_remote.h | 1 + > > > > 2 files changed, 37 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c > > > > index 4d51969..9b4c173 100644 > > > > --- a/fs/xfs/libxfs/xfs_attr_remote.c > > > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c > > > > @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove( > > > > xfs_dablk_t lblkno; > > > > int blkcnt; > > > > int error = 0; > > > > - int done = 0; > > > > + int retval = 0; > > > > trace_xfs_attr_rmtval_remove(args); > > > > @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove( > > > > */ > > > > lblkno = args->rmtblkno; > > > > blkcnt = args->rmtblkcnt; > > > > > > Er... I think these local variables can go away here, right? > > > > > > --D > > > > > > > - while (!done) { > > > > - error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt, > > > > - XFS_BMAPI_ATTRFORK, 1, &done); > > > > - if (error) > > > > - return error; > > > > - error = xfs_defer_finish(&args->trans); > > > > - if (error) > > > > - return error; > > > > + do { > > > > + retval = __xfs_attr_rmtval_remove(args); > > > > + if (retval && retval != EAGAIN) > > > > Also this has to be -EAGAIN. Amazingly, nothing in fstests blew up on > > this. > Ok, will fix! If you are running with the full set, it wouldnt trip over > anything because this whole function is removed by the end of the series. It's already in for-next, with numerous minor corrections for Carlos' kmem series. You might want to have a quick look around at the diff between xfs-5.9-merge-5 and xfs-5.9-merge-6 just in case I missed something... --D > Allison > > > > > --D > > > > > > + return retval; > > > > /* > > > > * Close out trans and start the next one in the chain. > > > > @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove( > > > > error = xfs_trans_roll_inode(&args->trans, args->dp); > > > > if (error) > > > > return error; > > > > - } > > > > + } while (retval == -EAGAIN); > > > > + > > > > return 0; > > > > } > > > > + > > > > +/* > > > > + * Remove the value associated with an attribute by deleting the out-of-line > > > > + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the > > > > + * transaction and re-call the function > > > > + */ > > > > +int > > > > +__xfs_attr_rmtval_remove( > > > > + struct xfs_da_args *args) > > > > +{ > > > > + int error, done; > > > > + > > > > + /* > > > > + * Unmap value blocks for this attr. > > > > + */ > > > > + error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno, > > > > + args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done); > > > > + if (error) > > > > + return error; > > > > + > > > > + error = xfs_defer_finish(&args->trans); > > > > + if (error) > > > > + return error; > > > > + > > > > + if (!done) > > > > + return -EAGAIN; > > > > + > > > > + return error; > > > > +} > > > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h > > > > index 3616e88..9eee615 100644 > > > > --- a/fs/xfs/libxfs/xfs_attr_remote.h > > > > +++ b/fs/xfs/libxfs/xfs_attr_remote.h > > > > @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args); > > > > int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map, > > > > xfs_buf_flags_t incore_flags); > > > > int xfs_attr_rmtval_invalidate(struct xfs_da_args *args); > > > > +int __xfs_attr_rmtval_remove(struct xfs_da_args *args); > > > > #endif /* __XFS_ATTR_REMOTE_H__ */ > > > > -- > > > > 2.7.4 > > > >
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c index 4d51969..9b4c173 100644 --- a/fs/xfs/libxfs/xfs_attr_remote.c +++ b/fs/xfs/libxfs/xfs_attr_remote.c @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove( xfs_dablk_t lblkno; int blkcnt; int error = 0; - int done = 0; + int retval = 0; trace_xfs_attr_rmtval_remove(args); @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove( */ lblkno = args->rmtblkno; blkcnt = args->rmtblkcnt; - while (!done) { - error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt, - XFS_BMAPI_ATTRFORK, 1, &done); - if (error) - return error; - error = xfs_defer_finish(&args->trans); - if (error) - return error; + do { + retval = __xfs_attr_rmtval_remove(args); + if (retval && retval != EAGAIN) + return retval; /* * Close out trans and start the next one in the chain. @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove( error = xfs_trans_roll_inode(&args->trans, args->dp); if (error) return error; - } + } while (retval == -EAGAIN); + return 0; } + +/* + * Remove the value associated with an attribute by deleting the out-of-line + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the + * transaction and re-call the function + */ +int +__xfs_attr_rmtval_remove( + struct xfs_da_args *args) +{ + int error, done; + + /* + * Unmap value blocks for this attr. + */ + error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno, + args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done); + if (error) + return error; + + error = xfs_defer_finish(&args->trans); + if (error) + return error; + + if (!done) + return -EAGAIN; + + return error; +} diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h index 3616e88..9eee615 100644 --- a/fs/xfs/libxfs/xfs_attr_remote.h +++ b/fs/xfs/libxfs/xfs_attr_remote.h @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args); int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map, xfs_buf_flags_t incore_flags); int xfs_attr_rmtval_invalidate(struct xfs_da_args *args); +int __xfs_attr_rmtval_remove(struct xfs_da_args *args); #endif /* __XFS_ATTR_REMOTE_H__ */