diff mbox series

[v5,05/14] xfs: Factor out new helper functions xfs_attr_rmtval_set

Message ID 20191212041513.13855-6-allison.henderson@oracle.com (mailing list archive)
State Superseded
Headers show
Series xfs: Delay Ready Attributes | expand

Commit Message

Allison Henderson Dec. 12, 2019, 4:15 a.m. UTC 
Break xfs_attr_rmtval_set into two helper functions
xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
xfs_attr_rmtval_set rolls the transaction between the
helpers, but delayed operations cannot.  We will use
the helpers later when constructing new delayed
attribute routines.

Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c | 71 +++++++++++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_attr_remote.h |  3 +-
 2 files changed, 56 insertions(+), 18 deletions(-)

Comments

Christoph Hellwig Dec. 24, 2019, 12:14 p.m. UTC | #1 
On Wed, Dec 11, 2019 at 09:15:04PM -0700, Allison Collins wrote:
> Break xfs_attr_rmtval_set into two helper functions
> xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
> xfs_attr_rmtval_set rolls the transaction between the
> helpers, but delayed operations cannot.  We will use
> the helpers later when constructing new delayed
> attribute routines.

Please use up the foll 72-ish characters for the changelog (also for
various other patches).

For the actual patch: can you keep the code in the order of the calling
conventions, that is the lower level functions up and
xfs_attr_rmtval_set at the bottom?  Also please keep the functions
static until callers show up (which nicely leads to the above order).
Allison Henderson Dec. 25, 2019, 5:43 p.m. UTC | #2 
On 12/24/19 5:14 AM, Christoph Hellwig wrote:
> On Wed, Dec 11, 2019 at 09:15:04PM -0700, Allison Collins wrote:
>> Break xfs_attr_rmtval_set into two helper functions
>> xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
>> xfs_attr_rmtval_set rolls the transaction between the
>> helpers, but delayed operations cannot.  We will use
>> the helpers later when constructing new delayed
>> attribute routines.
> 
> Please use up the foll 72-ish characters for the changelog (also for
> various other patches).
Hmm, in one of my older reviews, we thought the standard line wrap 
length was 68.  Maybe when more folks get back from holiday break, we 
can have more chime in here.

> 
> For the actual patch: can you keep the code in the order of the calling
> conventions, that is the lower level functions up and
> xfs_attr_rmtval_set at the bottom?  Also please keep the functions
> static until callers show up (which nicely leads to the above order).
> 

Sure, will do.

Allison
Brian Foster Jan. 6, 2020, 2:46 p.m. UTC | #3 
On Wed, Dec 25, 2019 at 10:43:15AM -0700, Allison Collins wrote:
> 
> 
> On 12/24/19 5:14 AM, Christoph Hellwig wrote:
> > On Wed, Dec 11, 2019 at 09:15:04PM -0700, Allison Collins wrote:
> > > Break xfs_attr_rmtval_set into two helper functions
> > > xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
> > > xfs_attr_rmtval_set rolls the transaction between the
> > > helpers, but delayed operations cannot.  We will use
> > > the helpers later when constructing new delayed
> > > attribute routines.
> > 
> > Please use up the foll 72-ish characters for the changelog (also for
> > various other patches).
> Hmm, in one of my older reviews, we thought the standard line wrap length
> was 68.  Maybe when more folks get back from holiday break, we can have more
> chime in here.
> 

I thought it was 68 as well (I think that qualifies as 72-ish" at
least), but the current commit logs still look short of that at a
glance. ;P

Brian

> > 
> > For the actual patch: can you keep the code in the order of the calling
> > conventions, that is the lower level functions up and
> > xfs_attr_rmtval_set at the bottom?  Also please keep the functions
> > static until callers show up (which nicely leads to the above order).
> > 
> 
> Sure, will do.
> 
> Allison
>
Allison Henderson Jan. 6, 2020, 6:29 p.m. UTC | #4 
On 1/6/20 7:46 AM, Brian Foster wrote:
> On Wed, Dec 25, 2019 at 10:43:15AM -0700, Allison Collins wrote:
>>
>>
>> On 12/24/19 5:14 AM, Christoph Hellwig wrote:
>>> On Wed, Dec 11, 2019 at 09:15:04PM -0700, Allison Collins wrote:
>>>> Break xfs_attr_rmtval_set into two helper functions
>>>> xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
>>>> xfs_attr_rmtval_set rolls the transaction between the
>>>> helpers, but delayed operations cannot.  We will use
>>>> the helpers later when constructing new delayed
>>>> attribute routines.
>>>
>>> Please use up the foll 72-ish characters for the changelog (also for
>>> various other patches).
>> Hmm, in one of my older reviews, we thought the standard line wrap length
>> was 68.  Maybe when more folks get back from holiday break, we can have more
>> chime in here.
>>
> 
> I thought it was 68 as well (I think that qualifies as 72-ish" at
> least), but the current commit logs still look short of that at a
> glance. ;P
> 
> Brian
Ok I doubled checked, the last few lines do wrap a little early, but the 
rest is correct for 68 because of the function names.  We should 
probably establish a number though.  In perusing around some of the 
other patches on the list, it looks to me like people are using 81?

Allison

> 
>>>
>>> For the actual patch: can you keep the code in the order of the calling
>>> conventions, that is the lower level functions up and
>>> xfs_attr_rmtval_set at the bottom?  Also please keep the functions
>>> static until callers show up (which nicely leads to the above order).
>>>
>>
>> Sure, will do.
>>
>> Allison
>>
>
Darrick J. Wong Jan. 6, 2020, 9:45 p.m. UTC | #5 
On Mon, Jan 06, 2020 at 11:29:29AM -0700, Allison Collins wrote:
> 
> 
> On 1/6/20 7:46 AM, Brian Foster wrote:
> > On Wed, Dec 25, 2019 at 10:43:15AM -0700, Allison Collins wrote:
> > > 
> > > 
> > > On 12/24/19 5:14 AM, Christoph Hellwig wrote:
> > > > On Wed, Dec 11, 2019 at 09:15:04PM -0700, Allison Collins wrote:
> > > > > Break xfs_attr_rmtval_set into two helper functions
> > > > > xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
> > > > > xfs_attr_rmtval_set rolls the transaction between the
> > > > > helpers, but delayed operations cannot.  We will use
> > > > > the helpers later when constructing new delayed
> > > > > attribute routines.
> > > > 
> > > > Please use up the foll 72-ish characters for the changelog (also for
> > > > various other patches).
> > > Hmm, in one of my older reviews, we thought the standard line wrap length
> > > was 68.  Maybe when more folks get back from holiday break, we can have more
> > > chime in here.
> > > 
> > 
> > I thought it was 68 as well (I think that qualifies as 72-ish" at
> > least), but the current commit logs still look short of that at a
> > glance. ;P
> > 
> > Brian
> Ok I doubled checked, the last few lines do wrap a little early, but the
> rest is correct for 68 because of the function names.  We should probably
> establish a number though.  In perusing around some of the other patches on
> the list, it looks to me like people are using 81?

I use 72 columns for emails and commit messages, and 79 for code.

Though to be honest that's just my editor settings; I'm sure interested
parties could find plenty of instances where my enforcement of even that
is totally lax --

I have enough of a difficult time finding all the subtle bugs and corner
case design problems in the kernel code (which will cause problems in
our users' lives) that so long as you're not obviously going past the
flaming red stripe that I told vim to put at column 80, I don't really
care (because maxcolumns errors don't usually cause data loss). :)

(Not trying to say that peoples' code is crap, I'm just laying out where
I put maxcolumns in my priority barrel.)

--D

> 
> Allison
> 
> > 
> > > > 
> > > > For the actual patch: can you keep the code in the order of the calling
> > > > conventions, that is the lower level functions up and
> > > > xfs_attr_rmtval_set at the bottom?  Also please keep the functions
> > > > static until callers show up (which nicely leads to the above order).
> > > > 
> > > 
> > > Sure, will do.
> > > 
> > > Allison
> > > 
> >
Dave Chinner Jan. 6, 2020, 11:33 p.m. UTC | #6 
On Mon, Jan 06, 2020 at 01:45:01PM -0800, Darrick J. Wong wrote:
> On Mon, Jan 06, 2020 at 11:29:29AM -0700, Allison Collins wrote:
> > 
> > 
> > On 1/6/20 7:46 AM, Brian Foster wrote:
> > > On Wed, Dec 25, 2019 at 10:43:15AM -0700, Allison Collins wrote:
> > > > 
> > > > 
> > > > On 12/24/19 5:14 AM, Christoph Hellwig wrote:
> > > > > On Wed, Dec 11, 2019 at 09:15:04PM -0700, Allison Collins wrote:
> > > > > > Break xfs_attr_rmtval_set into two helper functions
> > > > > > xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
> > > > > > xfs_attr_rmtval_set rolls the transaction between the
> > > > > > helpers, but delayed operations cannot.  We will use
> > > > > > the helpers later when constructing new delayed
> > > > > > attribute routines.
> > > > > 
> > > > > Please use up the foll 72-ish characters for the changelog (also for
> > > > > various other patches).
> > > > Hmm, in one of my older reviews, we thought the standard line wrap length
> > > > was 68.  Maybe when more folks get back from holiday break, we can have more
> > > > chime in here.
> > > > 
> > > 
> > > I thought it was 68 as well (I think that qualifies as 72-ish" at
> > > least), but the current commit logs still look short of that at a
> > > glance. ;P
> > > 
> > > Brian
> > Ok I doubled checked, the last few lines do wrap a little early, but the
> > rest is correct for 68 because of the function names.  We should probably
> > establish a number though.  In perusing around some of the other patches on
> > the list, it looks to me like people are using 81?
> 
> I use 72 columns for emails and commit messages, and 79 for code.

Typically 68-72 columns for commit messages, often 68 because git
log output adds a 4 space indent to the commit message and that
often gets quoted directly in email...

> Though to be honest that's just my editor settings; I'm sure interested
> parties could find plenty of instances where my enforcement of even that
> is totally lax --
> 
> I have enough of a difficult time finding all the subtle bugs and corner
> case design problems in the kernel code (which will cause problems in
> our users' lives) that so long as you're not obviously going past the
> flaming red stripe that I told vim to put at column 80, I don't really
> care (because maxcolumns errors don't usually cause data loss). :)

Yeah, I have the flaming red column set to 80 by default, 68 for
email and commit messages...

Cheers,

Dave.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 3e725c3..21f0001 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -22,6 +22,7 @@ 
 #include "xfs_attr_remote.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
+#include "xfs_attr_remote.h"
 
 #define ATTR_RMTVALUE_MAPSIZE	1	/* # of map entries at once */
 
@@ -433,34 +434,20 @@  xfs_attr_rmtval_set(
 	struct xfs_da_args	*args)
 {
 	struct xfs_inode	*dp = args->dp;
-	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_bmbt_irec	map;
 	xfs_dablk_t		lblkno;
-	xfs_fileoff_t		lfileoff = 0;
-	uint8_t			*src = args->value;
 	int			blkcnt;
-	int			valuelen;
 	int			nmap;
 	int			error;
-	int			offset = 0;
 
 	trace_xfs_attr_rmtval_set(args);
 
-	/*
-	 * Find a "hole" in the attribute address space large enough for
-	 * us to drop the new attribute's value into. Because CRC enable
-	 * attributes have headers, we can't just do a straight byte to FSB
-	 * conversion and have to take the header space into account.
-	 */
-	blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
-	error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
-						   XFS_ATTR_FORK);
+	error = xfs_attr_rmt_find_hole(args);
 	if (error)
 		return error;
 
-	args->rmtblkno = lblkno = (xfs_dablk_t)lfileoff;
-	args->rmtblkcnt = blkcnt;
-
+	blkcnt = args->rmtblkcnt;
+	lblkno = (xfs_dablk_t)args->rmtblkno;
 	/*
 	 * Roll through the "value", allocating blocks on disk as required.
 	 */
@@ -501,6 +488,56 @@  xfs_attr_rmtval_set(
 			return error;
 	}
 
+	return xfs_attr_rmtval_set_value(args);
+}
+
+
+/*
+ * Find a "hole" in the attribute address space large enough for us to drop the
+ * new attribute's value into
+ */
+int
+xfs_attr_rmt_find_hole(
+	struct xfs_da_args	*args)
+{
+	struct xfs_inode        *dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
+	int			error;
+	int			blkcnt;
+	xfs_fileoff_t		lfileoff = 0;
+
+	/*
+	 * Because CRC enable attributes have headers, we can't just do a
+	 * straight byte to FSB conversion and have to take the header space
+	 * into account.
+	 */
+	blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
+	error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
+						   XFS_ATTR_FORK);
+	if (error)
+		return error;
+
+	args->rmtblkno = (xfs_dablk_t)lfileoff;
+	args->rmtblkcnt = blkcnt;
+
+	return 0;
+}
+
+int
+xfs_attr_rmtval_set_value(
+	struct xfs_da_args	*args)
+{
+	struct xfs_inode	*dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_bmbt_irec	map;
+	xfs_dablk_t		lblkno;
+	uint8_t			*src = args->value;
+	int			blkcnt;
+	int			valuelen;
+	int			nmap;
+	int			error;
+	int			offset = 0;
+
 	/*
 	 * Roll through the "value", copying the attribute value to the
 	 * already-allocated blocks.  Blocks are written synchronously
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 9d20b66..cd7670d 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -11,5 +11,6 @@  int xfs_attr3_rmt_blocks(struct xfs_mount *mp, int attrlen);
 int xfs_attr_rmtval_get(struct xfs_da_args *args);
 int xfs_attr_rmtval_set(struct xfs_da_args *args);
 int xfs_attr_rmtval_remove(struct xfs_da_args *args);
-
+int xfs_attr_rmtval_set_value(struct xfs_da_args *args);
+int xfs_attr_rmt_find_hole(struct xfs_da_args *args);
 #endif /* __XFS_ATTR_REMOTE_H__ */