* [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay
@ 2016-11-30 22:33 Eric Sandeen
2016-12-01 12:15 ` Brian Foster
2016-12-05 16:21 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Eric Sandeen @ 2016-11-30 22:33 UTC (permalink / raw)
To: linux-xfs
When we create a new attribute, we first create a shortform
attribute, and try to fit the new attribute into it.
If that fails, we copy the (empty) attribute into a leaf attribute,
and do the copy again. Thus there can be a transient state where
we have an empty leaf attribute.
If we encounter this during log replay, the verifier will fail.
So add a test to ignore this part of the leaf attr verification
during log replay.
Thanks as usual to dchinner for spotting the problem.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 8ea91f3..2852521 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -253,6 +253,7 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
{
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_attr_leafblock *leaf = bp->b_addr;
+ struct xfs_perag *pag = bp->b_pag;
struct xfs_attr3_icleaf_hdr ichdr;
xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
@@ -273,7 +274,12 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
return false;
}
- if (ichdr.count == 0)
+ /*
+ * In recovery there is a transient state where count == 0 is valid
+ * because we may have transitioned an empty shortform attr to a leaf
+ * if the attr didn't fit in shortform.
+ */
+ if (pag && pag->pagf_init && ichdr.count == 0)
return false;
/* XXX: need to range check rest of attr header values */
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay
2016-11-30 22:33 [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay Eric Sandeen
@ 2016-12-01 12:15 ` Brian Foster
2016-12-05 20:31 ` Eric Sandeen
2016-12-05 16:21 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Brian Foster @ 2016-12-01 12:15 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Wed, Nov 30, 2016 at 04:33:15PM -0600, Eric Sandeen wrote:
> When we create a new attribute, we first create a shortform
> attribute, and try to fit the new attribute into it.
> If that fails, we copy the (empty) attribute into a leaf attribute,
> and do the copy again. Thus there can be a transient state where
> we have an empty leaf attribute.
>
> If we encounter this during log replay, the verifier will fail.
> So add a test to ignore this part of the leaf attr verification
> during log replay.
>
> Thanks as usual to dchinner for spotting the problem.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8ea91f3..2852521 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -253,6 +253,7 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_attr_leafblock *leaf = bp->b_addr;
> + struct xfs_perag *pag = bp->b_pag;
> struct xfs_attr3_icleaf_hdr ichdr;
>
> xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
> @@ -273,7 +274,12 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
> if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
> return false;
> }
> - if (ichdr.count == 0)
> + /*
> + * In recovery there is a transient state where count == 0 is valid
> + * because we may have transitioned an empty shortform attr to a leaf
> + * if the attr didn't fit in shortform.
> + */
> + if (pag && pag->pagf_init && ichdr.count == 0)
> return false;
Seems fine, but if the idea is to filter out failures during log
recovery, can we detect that state explicitly? E.g., check for some
combination of XLOG_ACTIVE_RECOVERY and/or XLOG_RECOVERY_NEEDED (or just
define and use a new flag/helper if necessary)?
Brian
>
> /* XXX: need to range check rest of attr header values */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay
2016-11-30 22:33 [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay Eric Sandeen
2016-12-01 12:15 ` Brian Foster
@ 2016-12-05 16:21 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2016-12-05 16:21 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Wed, Nov 30, 2016 at 04:33:15PM -0600, Eric Sandeen wrote:
> When we create a new attribute, we first create a shortform
> attribute, and try to fit the new attribute into it.
> If that fails, we copy the (empty) attribute into a leaf attribute,
> and do the copy again. Thus there can be a transient state where
> we have an empty leaf attribute.
>
> If we encounter this during log replay, the verifier will fail.
> So add a test to ignore this part of the leaf attr verification
> during log replay.
>
> Thanks as usual to dchinner for spotting the problem.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay
2016-12-01 12:15 ` Brian Foster
@ 2016-12-05 20:31 ` Eric Sandeen
2016-12-05 21:33 ` Brian Foster
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2016-12-05 20:31 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On 12/1/16 6:15 AM, Brian Foster wrote:
> On Wed, Nov 30, 2016 at 04:33:15PM -0600, Eric Sandeen wrote:
>> When we create a new attribute, we first create a shortform
>> attribute, and try to fit the new attribute into it.
>> If that fails, we copy the (empty) attribute into a leaf attribute,
>> and do the copy again. Thus there can be a transient state where
>> we have an empty leaf attribute.
>>
>> If we encounter this during log replay, the verifier will fail.
>> So add a test to ignore this part of the leaf attr verification
>> during log replay.
>>
>> Thanks as usual to dchinner for spotting the problem.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>> index 8ea91f3..2852521 100644
>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>> @@ -253,6 +253,7 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
>> {
>> struct xfs_mount *mp = bp->b_target->bt_mount;
>> struct xfs_attr_leafblock *leaf = bp->b_addr;
>> + struct xfs_perag *pag = bp->b_pag;
>> struct xfs_attr3_icleaf_hdr ichdr;
>>
>> xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>> @@ -273,7 +274,12 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
>> if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
>> return false;
>> }
>> - if (ichdr.count == 0)
>> + /*
>> + * In recovery there is a transient state where count == 0 is valid
>> + * because we may have transitioned an empty shortform attr to a leaf
>> + * if the attr didn't fit in shortform.
>> + */
>> + if (pag && pag->pagf_init && ichdr.count == 0)
>> return false;
>
> Seems fine, but if the idea is to filter out failures during log
> recovery, can we detect that state explicitly? E.g., check for some
> combination of XLOG_ACTIVE_RECOVERY and/or XLOG_RECOVERY_NEEDED (or just
> define and use a new flag/helper if necessary)?
Yeah, this is done in several other places; see xfs_allocbt_verify,
xfs_refcountbt_verify, xfs_rmapbt_verify and the comments in those.
Right now, XLOG_ACTIVE_RECOVERY is only used in the actual logging code.
I did consider that a helper w/ an explanation of why "pag && pag->pagf_init"
would be a good idea, though.
-Eric
> Brian
>
>>
>> /* XXX: need to range check rest of attr header values */
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay
2016-12-05 20:31 ` Eric Sandeen
@ 2016-12-05 21:33 ` Brian Foster
2016-12-05 21:45 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Brian Foster @ 2016-12-05 21:33 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Mon, Dec 05, 2016 at 02:31:32PM -0600, Eric Sandeen wrote:
> On 12/1/16 6:15 AM, Brian Foster wrote:
> > On Wed, Nov 30, 2016 at 04:33:15PM -0600, Eric Sandeen wrote:
> >> When we create a new attribute, we first create a shortform
> >> attribute, and try to fit the new attribute into it.
> >> If that fails, we copy the (empty) attribute into a leaf attribute,
> >> and do the copy again. Thus there can be a transient state where
> >> we have an empty leaf attribute.
> >>
> >> If we encounter this during log replay, the verifier will fail.
> >> So add a test to ignore this part of the leaf attr verification
> >> during log replay.
> >>
> >> Thanks as usual to dchinner for spotting the problem.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> >> index 8ea91f3..2852521 100644
> >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> >> @@ -253,6 +253,7 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
> >> {
> >> struct xfs_mount *mp = bp->b_target->bt_mount;
> >> struct xfs_attr_leafblock *leaf = bp->b_addr;
> >> + struct xfs_perag *pag = bp->b_pag;
> >> struct xfs_attr3_icleaf_hdr ichdr;
> >>
> >> xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
> >> @@ -273,7 +274,12 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
> >> if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
> >> return false;
> >> }
> >> - if (ichdr.count == 0)
> >> + /*
> >> + * In recovery there is a transient state where count == 0 is valid
> >> + * because we may have transitioned an empty shortform attr to a leaf
> >> + * if the attr didn't fit in shortform.
> >> + */
> >> + if (pag && pag->pagf_init && ichdr.count == 0)
> >> return false;
> >
> > Seems fine, but if the idea is to filter out failures during log
> > recovery, can we detect that state explicitly? E.g., check for some
> > combination of XLOG_ACTIVE_RECOVERY and/or XLOG_RECOVERY_NEEDED (or just
> > define and use a new flag/helper if necessary)?
>
> Yeah, this is done in several other places; see xfs_allocbt_verify,
> xfs_refcountbt_verify, xfs_rmapbt_verify and the comments in those.
>
Ok, but that doesn't necessarily look like the same thing. Those places
check for perag initialization because they check against values in the
perag data structure. Here we are just using the state to imply that log
recovery hasn't occurred yet.
What happens if for some unknown future reason we need an initialized
perag during/before log recovery and so decide to initialize it earlier
and invalidate it post-recovery (for e.g.) to deal with potential
inconsistencies? AFAICT the existing verifier logic should generally
work as expected, but this can become a landmine.
Granted, that isn't the case right now, it may never be, and you have an
r-b. So I guess it just depends on whether you reach my level of
paranoia. :)
Brian
> Right now, XLOG_ACTIVE_RECOVERY is only used in the actual logging code.
>
> I did consider that a helper w/ an explanation of why "pag && pag->pagf_init"
> would be a good idea, though.
>
> -Eric
>
> > Brian
> >
> >>
> >> /* XXX: need to range check rest of attr header values */
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay
2016-12-05 21:33 ` Brian Foster
@ 2016-12-05 21:45 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2016-12-05 21:45 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On 12/5/16 3:33 PM, Brian Foster wrote:
> On Mon, Dec 05, 2016 at 02:31:32PM -0600, Eric Sandeen wrote:
>> On 12/1/16 6:15 AM, Brian Foster wrote:
>>> On Wed, Nov 30, 2016 at 04:33:15PM -0600, Eric Sandeen wrote:
>>>> When we create a new attribute, we first create a shortform
>>>> attribute, and try to fit the new attribute into it.
>>>> If that fails, we copy the (empty) attribute into a leaf attribute,
>>>> and do the copy again. Thus there can be a transient state where
>>>> we have an empty leaf attribute.
>>>>
>>>> If we encounter this during log replay, the verifier will fail.
>>>> So add a test to ignore this part of the leaf attr verification
>>>> during log replay.
>>>>
>>>> Thanks as usual to dchinner for spotting the problem.
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>>>> index 8ea91f3..2852521 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>>>> @@ -253,6 +253,7 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
>>>> {
>>>> struct xfs_mount *mp = bp->b_target->bt_mount;
>>>> struct xfs_attr_leafblock *leaf = bp->b_addr;
>>>> + struct xfs_perag *pag = bp->b_pag;
>>>> struct xfs_attr3_icleaf_hdr ichdr;
>>>>
>>>> xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>>>> @@ -273,7 +274,12 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
>>>> if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
>>>> return false;
>>>> }
>>>> - if (ichdr.count == 0)
>>>> + /*
>>>> + * In recovery there is a transient state where count == 0 is valid
>>>> + * because we may have transitioned an empty shortform attr to a leaf
>>>> + * if the attr didn't fit in shortform.
>>>> + */
>>>> + if (pag && pag->pagf_init && ichdr.count == 0)
>>>> return false;
>>>
>>> Seems fine, but if the idea is to filter out failures during log
>>> recovery, can we detect that state explicitly? E.g., check for some
>>> combination of XLOG_ACTIVE_RECOVERY and/or XLOG_RECOVERY_NEEDED (or just
>>> define and use a new flag/helper if necessary)?
>>
>> Yeah, this is done in several other places; see xfs_allocbt_verify,
>> xfs_refcountbt_verify, xfs_rmapbt_verify and the comments in those.
>>
>
> Ok, but that doesn't necessarily look like the same thing. Those places
> check for perag initialization because they check against values in the
> perag data structure. Here we are just using the state to imply that log
> recovery hasn't occurred yet.
Yep :D
> What happens if for some unknown future reason we need an initialized
> perag during/before log recovery and so decide to initialize it earlier
> and invalidate it post-recovery (for e.g.) to deal with potential
> inconsistencies? AFAICT the existing verifier logic should generally
> work as expected, but this can become a landmine.
>
> Granted, that isn't the case right now, it may never be, and you have an
> r-b. So I guess it just depends on whether you reach my level of
> paranoia. :)
Oh, yeah, I raised an eyebrow for me too. But there was a precedent, and
I followed it. ;)
-Eric
> Brian
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-05 21:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-30 22:33 [PATCH] xfs: ignore leaf attr ichdr.count in verifier during log replay Eric Sandeen
2016-12-01 12:15 ` Brian Foster
2016-12-05 20:31 ` Eric Sandeen
2016-12-05 21:33 ` Brian Foster
2016-12-05 21:45 ` Eric Sandeen
2016-12-05 16:21 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).