[v7,12/19] xfs: Add helper function xfs_attr_rmtval_unmap
diff mbox series

Message ID 20200223020611.1802-13-allison.henderson@oracle.com
State New
Headers show
Series
  • xfs: Delayed Ready Attrs
Related show

Commit Message

Allison Collins Feb. 23, 2020, 2:06 a.m. UTC
This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
new transactions. We will use this later when we introduce delayed attributes

Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr_remote.h |  1 +
 2 files changed, 29 insertions(+)

Comments

Brian Foster Feb. 24, 2020, 1:40 p.m. UTC | #1
On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
> This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
> new transactions. We will use this later when we introduce delayed attributes
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 3de2eec..da40f85 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>  	}
>  	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 recall the function
> + */
> +int
> +xfs_attr_rmtval_unmap(
> +	struct xfs_da_args	*args)
> +{
> +	int	error, done;
> +
> +	/*
> +	 * Unmap value blocks for this attr.  This is similar to
> +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> +	 * for new transactions
> +	 */
> +	error = xfs_bunmapi(args->trans, args->dp,
> +		    args->rmtblkno, args->rmtblkcnt,
> +		    XFS_BMAPI_ATTRFORK, 1, &done);
> +	if (error)
> +		return error;
> +
> +	if (!done)
> +		return -EAGAIN;
> +
> +	return 0;
> +}

Hmm.. any reason this isn't a refactor of the existing remove function?
Just skipping to the end of the series, I see we leave the reference to
xfs_attr_rmtval_remove() (which no longer exists and so is not very
useful) in this comment as well as a stale function declaration in
xfs_attr_remote.h.

I haven't grokked how this is used yet, but it seems like it would be
more appropriate to lift out the transaction handling from the original
function as we have throughout the rest of the code. That could also
mean creating a temporary wrapper (i.e., rmtval_remove() calls
rmtval_unmap()) for the loop/transaction code that could be removed
later if it ends up unused. Either way is much easier to follow than
creating a (currently unused) replacement..

Brian

> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
>  #endif /* __XFS_ATTR_REMOTE_H__ */
> -- 
> 2.7.4
>
Allison Collins Feb. 24, 2020, 9:44 p.m. UTC | #2
On 2/24/20 6:40 AM, Brian Foster wrote:
> On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
>> This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
>> new transactions. We will use this later when we introduce delayed attributes
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>>   fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index 3de2eec..da40f85 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>>   	}
>>   	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 recall the function
>> + */
>> +int
>> +xfs_attr_rmtval_unmap(
>> +	struct xfs_da_args	*args)
>> +{
>> +	int	error, done;
>> +
>> +	/*
>> +	 * Unmap value blocks for this attr.  This is similar to
>> +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
>> +	 * for new transactions
>> +	 */
>> +	error = xfs_bunmapi(args->trans, args->dp,
>> +		    args->rmtblkno, args->rmtblkcnt,
>> +		    XFS_BMAPI_ATTRFORK, 1, &done);
>> +	if (error)
>> +		return error;
>> +
>> +	if (!done)
>> +		return -EAGAIN;
>> +
>> +	return 0;
>> +}
> 
> Hmm.. any reason this isn't a refactor of the existing remove function?
> Just skipping to the end of the series, I see we leave the reference to
> xfs_attr_rmtval_remove() (which no longer exists and so is not very
> useful) in this comment as well as a stale function declaration in
> xfs_attr_remote.h.
> 
> I haven't grokked how this is used yet, but it seems like it would be
> more appropriate to lift out the transaction handling from the original
> function as we have throughout the rest of the code. That could also
> mean creating a temporary wrapper (i.e., rmtval_remove() calls
> rmtval_unmap()) for the loop/transaction code that could be removed
> later if it ends up unused. Either way is much easier to follow than
> creating a (currently unused) replacement..
Yes, this came up in one of the other reviews.  I thought about it, but 
then decided against it.  xfs_attr_rmtval_remove disappears across 
patches 13 and 14.  The use of xfs_attr_rmtval_remove is replaced with 
xfs_attr_rmtval_unmap when we finally yank out all the transaction code. 
  The reason I dont want to do it all at once is because that would mean 
patches 12, 13, 14 and 19 would lump together to make the swap 
instantaneous in once patch.

I've been getting feedback that the set is really complicated, so I've 
been trying to find a way to organize it to help make it easier to 
review.  So I thought isolating 13 and 14 to just the state machine 
would help.  Thus I decided to keep patch 12 separate to take as much 
craziness out of 13 and 14 as possible.  Patches 12 and 19 seem like 
otherwise easy things for people to look at.  Let me know your thoughts 
on this. :-)

You are right about the stale comment though, I missed it while going 
back over the commentary at the top.  Will fix.

Allison

> 
> Brian
> 
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
>> index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
>>   #endif /* __XFS_ATTR_REMOTE_H__ */
>> -- 
>> 2.7.4
>>
>
Dave Chinner Feb. 25, 2020, 7:21 a.m. UTC | #3
On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
> This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
> new transactions. We will use this later when we introduce delayed attributes

68-72 columns...

> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 3de2eec..da40f85 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>  	}
>  	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 recall the function
> + */
> +int
> +xfs_attr_rmtval_unmap(
> +	struct xfs_da_args	*args)
> +{
> +	int	error, done;

Best to initialise done to zero here.

> +
> +	/*
> +	 * Unmap value blocks for this attr.  This is similar to
> +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> +	 * for new transactions
> +	 */
> +	error = xfs_bunmapi(args->trans, args->dp,
> +		    args->rmtblkno, args->rmtblkcnt,
> +		    XFS_BMAPI_ATTRFORK, 1, &done);

Got 80 columns for code, please use them all :)

> +	if (error)
> +		return error;
> +
> +	if (!done)
> +		return -EAGAIN;

Hmmm, I guess I'm missing the context at this point: why not just pass the done
variable all the way back to the caller that will be looping on this function
call?

That turns this function into:

int
xfs_attr_rmtval_unmap(
	struct xfs_da_args      *args,
	bool			*done)
{
	return xfs_bunmapi(args->trans, args->dp, args->rmtblkno,
				args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, done);
}

Cheers,

Dave.
Brian Foster Feb. 25, 2020, 1:27 p.m. UTC | #4
On Mon, Feb 24, 2020 at 02:44:14PM -0700, Allison Collins wrote:
> 
> 
> On 2/24/20 6:40 AM, Brian Foster wrote:
> > On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
> > > This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
> > > new transactions. We will use this later when we introduce delayed attributes
> > > 
> > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > ---
> > >   fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
> > >   fs/xfs/libxfs/xfs_attr_remote.h |  1 +
> > >   2 files changed, 29 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > > index 3de2eec..da40f85 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > > @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
> > >   	}
> > >   	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 recall the function
> > > + */
> > > +int
> > > +xfs_attr_rmtval_unmap(
> > > +	struct xfs_da_args	*args)
> > > +{
> > > +	int	error, done;
> > > +
> > > +	/*
> > > +	 * Unmap value blocks for this attr.  This is similar to
> > > +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> > > +	 * for new transactions
> > > +	 */
> > > +	error = xfs_bunmapi(args->trans, args->dp,
> > > +		    args->rmtblkno, args->rmtblkcnt,
> > > +		    XFS_BMAPI_ATTRFORK, 1, &done);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	if (!done)
> > > +		return -EAGAIN;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Hmm.. any reason this isn't a refactor of the existing remove function?
> > Just skipping to the end of the series, I see we leave the reference to
> > xfs_attr_rmtval_remove() (which no longer exists and so is not very
> > useful) in this comment as well as a stale function declaration in
> > xfs_attr_remote.h.
> > 
> > I haven't grokked how this is used yet, but it seems like it would be
> > more appropriate to lift out the transaction handling from the original
> > function as we have throughout the rest of the code. That could also
> > mean creating a temporary wrapper (i.e., rmtval_remove() calls
> > rmtval_unmap()) for the loop/transaction code that could be removed
> > later if it ends up unused. Either way is much easier to follow than
> > creating a (currently unused) replacement..
> Yes, this came up in one of the other reviews.  I thought about it, but then
> decided against it.  xfs_attr_rmtval_remove disappears across patches 13 and
> 14.  The use of xfs_attr_rmtval_remove is replaced with
> xfs_attr_rmtval_unmap when we finally yank out all the transaction code.
> The reason I dont want to do it all at once is because that would mean
> patches 12, 13, 14 and 19 would lump together to make the swap instantaneous
> in once patch.
> 

Hmm.. I don't think we're talking about the same thing. If
xfs_attr_rmtval_remove() was broken down into two functions such that
one of the two looks exactly like the _unmap() variant, can't we just
remove the other half when it becomes unused and allow the _remove()
variant to exist with the implementation of _unmap() proposed here? This
seems fairly mechanical to me..

> I've been getting feedback that the set is really complicated, so I've been
> trying to find a way to organize it to help make it easier to review.  So I
> thought isolating 13 and 14 to just the state machine would help.  Thus I
> decided to keep patch 12 separate to take as much craziness out of 13 and 14
> as possible.  Patches 12 and 19 seem like otherwise easy things for people
> to look at.  Let me know your thoughts on this. :-)
> 

I think doing as much refactoring of existing code as early as possible
will go a long way towards simplifying the complexity around the
introduction of the state bits.

Brian

> You are right about the stale comment though, I missed it while going back
> over the commentary at the top.  Will fix.
> 
> Allison
> 
> > 
> > Brian
> > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> > > index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
> > >   #endif /* __XFS_ATTR_REMOTE_H__ */
> > > -- 
> > > 2.7.4
> > > 
> > 
>
Allison Collins Feb. 25, 2020, 11:27 p.m. UTC | #5
On 2/25/20 12:21 AM, Dave Chinner wrote:
> On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
>> This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
>> new transactions. We will use this later when we introduce delayed attributes
> 
> 68-72 columns...
> 
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>>   fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index 3de2eec..da40f85 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>>   	}
>>   	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 recall the function
>> + */
>> +int
>> +xfs_attr_rmtval_unmap(
>> +	struct xfs_da_args	*args)
>> +{
>> +	int	error, done;
> 
> Best to initialise done to zero here.
> 
>> +
>> +	/*
>> +	 * Unmap value blocks for this attr.  This is similar to
>> +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
>> +	 * for new transactions
>> +	 */
>> +	error = xfs_bunmapi(args->trans, args->dp,
>> +		    args->rmtblkno, args->rmtblkcnt,
>> +		    XFS_BMAPI_ATTRFORK, 1, &done);
> 
> Got 80 columns for code, please use them all :)
> 
>> +	if (error)
>> +		return error;
>> +
>> +	if (!done)
>> +		return -EAGAIN;
> 
> Hmmm, I guess I'm missing the context at this point: why not just pass the done
> variable all the way back to the caller that will be looping on this function
> call?
> 
> That turns this function into:
> 
> int
> xfs_attr_rmtval_unmap(
> 	struct xfs_da_args      *args,
> 	bool			*donec
> {
> 	return xfs_bunmapi(args->trans, args->dp, args->rmtblkno,
> 				args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, done);
> }
Well, this is a helper we talked about adding in the v5 review ([PATCH 
v5 13/14] xfs: Add delay ready attr remove routines)  In v5, I did not 
have this helper function, and just open coded xfs_attr_rmtval_remove in 
the middle of the calling function (xfs_attr_remove_iter), except with 
out the transaction.  Then later we identified it as an area we could 
modularize.  It's kind of hard to see the bigger picture with out the 
states being here yet.  But really what we're trying to do is modularize 
everything that shows up between the states later in the set.

Hope that helps a bit!
Allison

> 
> Cheers,
> 
> Dave.
>
Allison Collins Feb. 26, 2020, 3:29 a.m. UTC | #6
On 2/25/20 6:27 AM, Brian Foster wrote:
> On Mon, Feb 24, 2020 at 02:44:14PM -0700, Allison Collins wrote:
>>
>>
>> On 2/24/20 6:40 AM, Brian Foster wrote:
>>> On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
>>>> This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
>>>> new transactions. We will use this later when we introduce delayed attributes
>>>>
>>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>>> ---
>>>>    fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>>>>    fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>>>>    2 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> index 3de2eec..da40f85 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>>>>    	}
>>>>    	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 recall the function
>>>> + */
>>>> +int
>>>> +xfs_attr_rmtval_unmap(
>>>> +	struct xfs_da_args	*args)
>>>> +{
>>>> +	int	error, done;
>>>> +
>>>> +	/*
>>>> +	 * Unmap value blocks for this attr.  This is similar to
>>>> +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
>>>> +	 * for new transactions
>>>> +	 */
>>>> +	error = xfs_bunmapi(args->trans, args->dp,
>>>> +		    args->rmtblkno, args->rmtblkcnt,
>>>> +		    XFS_BMAPI_ATTRFORK, 1, &done);
>>>> +	if (error)
>>>> +		return error;
>>>> +
>>>> +	if (!done)
>>>> +		return -EAGAIN;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Hmm.. any reason this isn't a refactor of the existing remove function?
>>> Just skipping to the end of the series, I see we leave the reference to
>>> xfs_attr_rmtval_remove() (which no longer exists and so is not very
>>> useful) in this comment as well as a stale function declaration in
>>> xfs_attr_remote.h.
>>>
>>> I haven't grokked how this is used yet, but it seems like it would be
>>> more appropriate to lift out the transaction handling from the original
>>> function as we have throughout the rest of the code. That could also
>>> mean creating a temporary wrapper (i.e., rmtval_remove() calls
>>> rmtval_unmap()) for the loop/transaction code that could be removed
>>> later if it ends up unused. Either way is much easier to follow than
>>> creating a (currently unused) replacement..
>> Yes, this came up in one of the other reviews.  I thought about it, but then
>> decided against it.  xfs_attr_rmtval_remove disappears across patches 13 and
>> 14.  The use of xfs_attr_rmtval_remove is replaced with
>> xfs_attr_rmtval_unmap when we finally yank out all the transaction code.
>> The reason I dont want to do it all at once is because that would mean
>> patches 12, 13, 14 and 19 would lump together to make the swap instantaneous
>> in once patch.
>>
> 
> Hmm.. I don't think we're talking about the same thing. If
> xfs_attr_rmtval_remove() was broken down into two functions such that
> one of the two looks exactly like the _unmap() variant, can't we just
> remove the other half when it becomes unused and allow the _remove()
> variant to exist with the implementation of _unmap() proposed here? This
> seems fairly mechanical to me..
Oh, I see what you mean.  No, I had done a review of the uses of 
xfs_attr_rmtval_remove and realized that it appears in both the set and 
remove paths. We use it in the set path when we're doing a rename, and 
need to remove the old attr.

So in patch 13, we replace xfs_attr_rmtval_remove with 
xfs_attr_rmtval_unmap for the remove routines.  But it's still in the 
set routines.  So we cant take it away yet.  Once we get through patch 
14, it is no longer used and we can remove it.  Did that make sense?

> 
>> I've been getting feedback that the set is really complicated, so I've been
>> trying to find a way to organize it to help make it easier to review.  So I
>> thought isolating 13 and 14 to just the state machine would help.  Thus I
>> decided to keep patch 12 separate to take as much craziness out of 13 and 14
>> as possible.  Patches 12 and 19 seem like otherwise easy things for people
>> to look at.  Let me know your thoughts on this. :-)
>>
> 
> I think doing as much refactoring of existing code as early as possible
> will go a long way towards simplifying the complexity around the
> introduction of the state bits.

Alrighty, I was thinking that if I reordered things such that 
modularizing appeared at the end of the set, that it would help people 
to see it in context with the states.  But it sounds like people like it 
the other way around, it's easy enough to put back.  Though I think we 
may still be stuck with 12 and 19 being apart unless 13 and 14 come 
together.  :-(

Allison

> 
> Brian
> 
>> You are right about the stale comment though, I missed it while going back
>> over the commentary at the top.  Will fix.
>>
>> Allison
>>
>>>
>>> Brian
>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
>>>> index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
>>>>    #endif /* __XFS_ATTR_REMOTE_H__ */
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>
>
Brian Foster Feb. 26, 2020, 1:47 p.m. UTC | #7
On Tue, Feb 25, 2020 at 08:29:06PM -0700, Allison Collins wrote:
> 
> 
> On 2/25/20 6:27 AM, Brian Foster wrote:
> > On Mon, Feb 24, 2020 at 02:44:14PM -0700, Allison Collins wrote:
> > > 
> > > 
> > > On 2/24/20 6:40 AM, Brian Foster wrote:
> > > > On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
> > > > > This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
> > > > > new transactions. We will use this later when we introduce delayed attributes
> > > > > 
> > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > > > ---
> > > > >    fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
> > > > >    fs/xfs/libxfs/xfs_attr_remote.h |  1 +
> > > > >    2 files changed, 29 insertions(+)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > > > > index 3de2eec..da40f85 100644
> > > > > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > > > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > > > > @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
> > > > >    	}
> > > > >    	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 recall the function
> > > > > + */
> > > > > +int
> > > > > +xfs_attr_rmtval_unmap(
> > > > > +	struct xfs_da_args	*args)
> > > > > +{
> > > > > +	int	error, done;
> > > > > +
> > > > > +	/*
> > > > > +	 * Unmap value blocks for this attr.  This is similar to
> > > > > +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> > > > > +	 * for new transactions
> > > > > +	 */
> > > > > +	error = xfs_bunmapi(args->trans, args->dp,
> > > > > +		    args->rmtblkno, args->rmtblkcnt,
> > > > > +		    XFS_BMAPI_ATTRFORK, 1, &done);
> > > > > +	if (error)
> > > > > +		return error;
> > > > > +
> > > > > +	if (!done)
> > > > > +		return -EAGAIN;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > Hmm.. any reason this isn't a refactor of the existing remove function?
> > > > Just skipping to the end of the series, I see we leave the reference to
> > > > xfs_attr_rmtval_remove() (which no longer exists and so is not very
> > > > useful) in this comment as well as a stale function declaration in
> > > > xfs_attr_remote.h.
> > > > 
> > > > I haven't grokked how this is used yet, but it seems like it would be
> > > > more appropriate to lift out the transaction handling from the original
> > > > function as we have throughout the rest of the code. That could also
> > > > mean creating a temporary wrapper (i.e., rmtval_remove() calls
> > > > rmtval_unmap()) for the loop/transaction code that could be removed
> > > > later if it ends up unused. Either way is much easier to follow than
> > > > creating a (currently unused) replacement..
> > > Yes, this came up in one of the other reviews.  I thought about it, but then
> > > decided against it.  xfs_attr_rmtval_remove disappears across patches 13 and
> > > 14.  The use of xfs_attr_rmtval_remove is replaced with
> > > xfs_attr_rmtval_unmap when we finally yank out all the transaction code.
> > > The reason I dont want to do it all at once is because that would mean
> > > patches 12, 13, 14 and 19 would lump together to make the swap instantaneous
> > > in once patch.
> > > 
> > 
> > Hmm.. I don't think we're talking about the same thing. If
> > xfs_attr_rmtval_remove() was broken down into two functions such that
> > one of the two looks exactly like the _unmap() variant, can't we just
> > remove the other half when it becomes unused and allow the _remove()
> > variant to exist with the implementation of _unmap() proposed here? This
> > seems fairly mechanical to me..
> Oh, I see what you mean.  No, I had done a review of the uses of
> xfs_attr_rmtval_remove and realized that it appears in both the set and
> remove paths. We use it in the set path when we're doing a rename, and need
> to remove the old attr.
> 
> So in patch 13, we replace xfs_attr_rmtval_remove with xfs_attr_rmtval_unmap
> for the remove routines.  But it's still in the set routines.  So we cant
> take it away yet.  Once we get through patch 14, it is no longer used and we
> can remove it.  Did that make sense?
> 

I don't think you need to take it away in this patch. What I'm
suggesting is breaking existing code into something like this:

int
__xfs_attr_rmtval_remove()
{
	...
	error = xfs_bunmapi(...);
	...

	if (!done)
		return -EAGAIN;

	...
}

int
xfs_attr_rmtval_remove()
{
	error = xfs_attr_rmtval_invalidate();

	do {
		error = __xfs_attr_rmtval_remove();
		...
		xfs_defer_finish();
		...
		xfs_trans_roll_inode(...);
	} while (-EAGAIN);

	...
}

So we can continue to use both as needed without duplication and remove
the bits that become unused as the series progresses. Eventually we
could rename __xfs_attr_rmtval_remove() back to xfs_attr_rmtval_remove()
to maintain that it's essentially the same function and only the
transaction rolling bits are being factored out and eventually removed.

Brian

> > 
> > > I've been getting feedback that the set is really complicated, so I've been
> > > trying to find a way to organize it to help make it easier to review.  So I
> > > thought isolating 13 and 14 to just the state machine would help.  Thus I
> > > decided to keep patch 12 separate to take as much craziness out of 13 and 14
> > > as possible.  Patches 12 and 19 seem like otherwise easy things for people
> > > to look at.  Let me know your thoughts on this. :-)
> > > 
> > 
> > I think doing as much refactoring of existing code as early as possible
> > will go a long way towards simplifying the complexity around the
> > introduction of the state bits.
> 
> Alrighty, I was thinking that if I reordered things such that modularizing
> appeared at the end of the set, that it would help people to see it in
> context with the states.  But it sounds like people like it the other way
> around, it's easy enough to put back.  Though I think we may still be stuck
> with 12 and 19 being apart unless 13 and 14 come together.  :-(
> 
> Allison
> 
> > 
> > Brian
> > 
> > > You are right about the stale comment though, I missed it while going back
> > > over the commentary at the top.  Will fix.
> > > 
> > > Allison
> > > 
> > > > 
> > > > Brian
> > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> > > > > index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
> > > > >    #endif /* __XFS_ATTR_REMOTE_H__ */
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > 
> > > 
> > 
>
Chandan Rajendra Feb. 28, 2020, 2:22 p.m. UTC | #8
On Sunday, February 23, 2020 7:36 AM Allison Collins wrote: 
> This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
> new transactions. We will use this later when we introduce delayed attributes
>
I don't see any logical errors.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 3de2eec..da40f85 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>  	}
>  	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 recall the function
> + */
> +int
> +xfs_attr_rmtval_unmap(
> +	struct xfs_da_args	*args)
> +{
> +	int	error, done;
> +
> +	/*
> +	 * Unmap value blocks for this attr.  This is similar to
> +	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> +	 * for new transactions
> +	 */
> +	error = xfs_bunmapi(args->trans, args->dp,
> +		    args->rmtblkno, args->rmtblkcnt,
> +		    XFS_BMAPI_ATTRFORK, 1, &done);
> +	if (error)
> +		return error;
> +
> +	if (!done)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
>  #endif /* __XFS_ATTR_REMOTE_H__ */
>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 3de2eec..da40f85 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -711,3 +711,31 @@  xfs_attr_rmtval_remove(
 	}
 	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 recall the function
+ */
+int
+xfs_attr_rmtval_unmap(
+	struct xfs_da_args	*args)
+{
+	int	error, done;
+
+	/*
+	 * Unmap value blocks for this attr.  This is similar to
+	 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
+	 * for new transactions
+	 */
+	error = xfs_bunmapi(args->trans, args->dp,
+		    args->rmtblkno, args->rmtblkcnt,
+		    XFS_BMAPI_ATTRFORK, 1, &done);
+	if (error)
+		return error;
+
+	if (!done)
+		return -EAGAIN;
+
+	return 0;
+}
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index eff5f95..e06299a 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_unmap(struct xfs_da_args *args);
 #endif /* __XFS_ATTR_REMOTE_H__ */