diff mbox series

[v2,2/2] ocfs2: fix NULL pointer dereference in ocfs2_abort_trigger()

Message ID 20240602112045.1112708-1-joseph.qi@linux.alibaba.com (mailing list archive)
State New
Headers show
Series None | expand

Commit Message

Joseph Qi June 2, 2024, 11:20 a.m. UTC
bdev->bd_super has been removed and commit 8887b94d9322 change the usage
from bdev->bd_super to b_assoc_map->host->i_sb. Since ocfs2 hasn't set
bh->b_assoc_map, it will trigger NULL pointer dereference when calling
into ocfs2_abort_trigger().

Actually this was pointed out in history, see commit 74e364ad1b13. But
I've made a mistake when reviewing commit 8887b94d9322 and then
re-introduce this regression.

Since we cannot revive bdev in buffer head, so fix this issue by
initializing all types of ocfs2 triggers when fill super, and then get
the specific ocfs2 trigger from ocfs2_caching_info when access journal.

Fixes: 8887b94d9322 ("ocfs2: stop using bdev->bd_super for journal error logging")
Cc: stable@vger.kernel.org # 6.6+
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/journal.c | 182 +++++++++++++++++++++++++--------------------
 fs/ocfs2/ocfs2.h   |  27 +++++++
 fs/ocfs2/super.c   |   4 +-
 3 files changed, 131 insertions(+), 82 deletions(-)

Comments

Heming Zhao June 3, 2024, 3:28 a.m. UTC | #1
Hello,

This patch looks good to me. please see my comment below.

On 6/2/24 19:20, Joseph Qi wrote:
> bdev->bd_super has been removed and commit 8887b94d9322 change the usage
> from bdev->bd_super to b_assoc_map->host->i_sb. Since ocfs2 hasn't set
> bh->b_assoc_map, it will trigger NULL pointer dereference when calling
> into ocfs2_abort_trigger().
> 
> Actually this was pointed out in history, see commit 74e364ad1b13. But
> I've made a mistake when reviewing commit 8887b94d9322 and then
> re-introduce this regression.
> 
> Since we cannot revive bdev in buffer head, so fix this issue by
> initializing all types of ocfs2 triggers when fill super, and then get
> the specific ocfs2 trigger from ocfs2_caching_info when access journal.
> 
> Fixes: 8887b94d9322 ("ocfs2: stop using bdev->bd_super for journal error logging")
> Cc: stable@vger.kernel.org # 6.6+
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
>   fs/ocfs2/journal.c | 182 +++++++++++++++++++++++++--------------------
>   fs/ocfs2/ocfs2.h   |  27 +++++++
>   fs/ocfs2/super.c   |   4 +-
>   3 files changed, 131 insertions(+), 82 deletions(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index 27c7683c7d3f..86807086b2df 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -479,12 +479,6 @@ int ocfs2_allocate_extend_trans(handle_t *handle, int thresh)
>   	return status;
>   }
>   
> -
> -struct ocfs2_triggers {
> -	struct jbd2_buffer_trigger_type	ot_triggers;
> -	int				ot_offset;
> -};
> -
>   static inline struct ocfs2_triggers *to_ocfs2_trigger(struct jbd2_buffer_trigger_type *triggers)
>   {
>   	return container_of(triggers, struct ocfs2_triggers, ot_triggers);
> @@ -548,85 +542,76 @@ static void ocfs2_db_frozen_trigger(struct jbd2_buffer_trigger_type *triggers,
>   static void ocfs2_abort_trigger(struct jbd2_buffer_trigger_type *triggers,
>   				struct buffer_head *bh)
>   {
> +	struct ocfs2_triggers *ot = to_ocfs2_trigger(triggers);
> +
>   	mlog(ML_ERROR,
>   	     "ocfs2_abort_trigger called by JBD2.  bh = 0x%lx, "
>   	     "bh->b_blocknr = %llu\n",
>   	     (unsigned long)bh,
>   	     (unsigned long long)bh->b_blocknr);
>   
> -	ocfs2_error(bh->b_assoc_map->host->i_sb,
> +	ocfs2_error(ot->sb,
>   		    "JBD2 has aborted our journal, ocfs2 cannot continue\n");
>   }
>   
> -static struct ocfs2_triggers di_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_dinode, i_check),
> -};
> -
> -static struct ocfs2_triggers eb_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_extent_block, h_check),
> -};
> -
> -static struct ocfs2_triggers rb_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_refcount_block, rf_check),
> -};
> -
> -static struct ocfs2_triggers gd_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_group_desc, bg_check),
> -};
> -
> -static struct ocfs2_triggers db_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_db_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -};
> +static void ocfs2_setup_csum_triggers(struct super_block *sb,
> +				      enum ocfs2_journal_trigger_type type,
> +				      struct ocfs2_triggers *ot)
> +{
> +	BUG_ON(type >= OCFS2_JOURNAL_TRIGGER_COUNT);
>   
> -static struct ocfs2_triggers xb_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_xattr_block, xb_check),
> -};
> +	switch (type) {
> +	case OCFS2_JTR_DI:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_dinode, i_check);
> +		break;
> +	case OCFS2_JTR_EB:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_extent_block, h_check);
> +		break;
> +	case OCFS2_JTR_RB:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_refcount_block, rf_check);
> +		break;
> +	case OCFS2_JTR_GD:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_group_desc, bg_check);
> +		break;
> +	case OCFS2_JTR_DB:
> +		ot->ot_triggers.t_frozen = ocfs2_db_frozen_trigger;
> +		break;
> +	case OCFS2_JTR_XB:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_xattr_block, xb_check);
> +		break;
> +	case OCFS2_JTR_DQ:
> +		ot->ot_triggers.t_frozen = ocfs2_dq_frozen_trigger;
> +		break;
> +	case OCFS2_JTR_DR:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_dx_root_block, dr_check);
> +		break;
> +	case OCFS2_JTR_DL:
> +		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
> +		ot->ot_offset = offsetof(struct ocfs2_dx_leaf, dl_check);
> +		break;
> +	case OCFS2_JTR_NONE:
> +		/* To make compiler happy... */
> +		return;
> +	}
>   
> -static struct ocfs2_triggers dq_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_dq_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -};
> +	ot->ot_triggers.t_abort = ocfs2_abort_trigger;
> +	ot->sb = sb;
> +}
>   
> -static struct ocfs2_triggers dr_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_dx_root_block, dr_check),
> -};
> +void ocfs2_initialize_journal_triggers(struct super_block *sb,
> +				       struct ocfs2_triggers triggers[])
> +{
> +	enum ocfs2_journal_trigger_type type;
>   
> -static struct ocfs2_triggers dl_triggers = {
> -	.ot_triggers = {
> -		.t_frozen = ocfs2_frozen_trigger,
> -		.t_abort = ocfs2_abort_trigger,
> -	},
> -	.ot_offset	= offsetof(struct ocfs2_dx_leaf, dl_check),
> -};
> +	for (type = OCFS2_JTR_DI; type < OCFS2_JOURNAL_TRIGGER_COUNT; type++)
> +		ocfs2_setup_csum_triggers(sb, type, &triggers[type]);
> +}
>   
>   static int __ocfs2_journal_access(handle_t *handle,
>   				  struct ocfs2_caching_info *ci,
> @@ -708,56 +693,91 @@ static int __ocfs2_journal_access(handle_t *handle,
>   int ocfs2_journal_access_di(handle_t *handle, struct ocfs2_caching_info *ci,
>   			    struct buffer_head *bh, int type)
>   {
> -	return __ocfs2_journal_access(handle, ci, bh, &di_triggers, type);
> +	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
> +
> +	return __ocfs2_journal_access(handle, ci, bh,
> +				      &osb->s_journal_triggers[OCFS2_JTR_DI],
> +				      type);

Legacy code uses a static define '&di_triggers', new code does some pointer
converting to fetch 'trigger'. I am not GCC expert, but in my view, it's better
to add the 'inline' attribute for some (not every) ocfs2_journal_access_xx()
function. e.g: ocfs2_journal_access_di().
I checked some ocfs2_journal_access_di() callers, most of callers already
have 'osb'. If GCC could inline this functions and remove the duplicated
pointer conversions, there could save some cpu time.

Thanks
Joseph Qi June 3, 2024, 3:43 a.m. UTC | #2
On 6/3/24 11:28 AM, Heming Zhao wrote:
> Hello,
> 
> This patch looks good to me. please see my comment below.
> 
> On 6/2/24 19:20, Joseph Qi wrote:
>> bdev->bd_super has been removed and commit 8887b94d9322 change the usage
>> from bdev->bd_super to b_assoc_map->host->i_sb. Since ocfs2 hasn't set
>> bh->b_assoc_map, it will trigger NULL pointer dereference when calling
>> into ocfs2_abort_trigger().
>>
>> Actually this was pointed out in history, see commit 74e364ad1b13. But
>> I've made a mistake when reviewing commit 8887b94d9322 and then
>> re-introduce this regression.
>>
>> Since we cannot revive bdev in buffer head, so fix this issue by
>> initializing all types of ocfs2 triggers when fill super, and then get
>> the specific ocfs2 trigger from ocfs2_caching_info when access journal.
>>
>> Fixes: 8887b94d9322 ("ocfs2: stop using bdev->bd_super for journal error logging")
>> Cc: stable@vger.kernel.org # 6.6+
>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
>> ---
>>   fs/ocfs2/journal.c | 182 +++++++++++++++++++++++++--------------------
>>   fs/ocfs2/ocfs2.h   |  27 +++++++
>>   fs/ocfs2/super.c   |   4 +-
>>   3 files changed, 131 insertions(+), 82 deletions(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index 27c7683c7d3f..86807086b2df 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -479,12 +479,6 @@ int ocfs2_allocate_extend_trans(handle_t *handle, int thresh)
>>       return status;
>>   }
>>   -
>> -struct ocfs2_triggers {
>> -    struct jbd2_buffer_trigger_type    ot_triggers;
>> -    int                ot_offset;
>> -};
>> -
>>   static inline struct ocfs2_triggers *to_ocfs2_trigger(struct jbd2_buffer_trigger_type *triggers)
>>   {
>>       return container_of(triggers, struct ocfs2_triggers, ot_triggers);
>> @@ -548,85 +542,76 @@ static void ocfs2_db_frozen_trigger(struct jbd2_buffer_trigger_type *triggers,
>>   static void ocfs2_abort_trigger(struct jbd2_buffer_trigger_type *triggers,
>>                   struct buffer_head *bh)
>>   {
>> +    struct ocfs2_triggers *ot = to_ocfs2_trigger(triggers);
>> +
>>       mlog(ML_ERROR,
>>            "ocfs2_abort_trigger called by JBD2.  bh = 0x%lx, "
>>            "bh->b_blocknr = %llu\n",
>>            (unsigned long)bh,
>>            (unsigned long long)bh->b_blocknr);
>>   -    ocfs2_error(bh->b_assoc_map->host->i_sb,
>> +    ocfs2_error(ot->sb,
>>               "JBD2 has aborted our journal, ocfs2 cannot continue\n");
>>   }
>>   -static struct ocfs2_triggers di_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_dinode, i_check),
>> -};
>> -
>> -static struct ocfs2_triggers eb_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_extent_block, h_check),
>> -};
>> -
>> -static struct ocfs2_triggers rb_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_refcount_block, rf_check),
>> -};
>> -
>> -static struct ocfs2_triggers gd_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_group_desc, bg_check),
>> -};
>> -
>> -static struct ocfs2_triggers db_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_db_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -};
>> +static void ocfs2_setup_csum_triggers(struct super_block *sb,
>> +                      enum ocfs2_journal_trigger_type type,
>> +                      struct ocfs2_triggers *ot)
>> +{
>> +    BUG_ON(type >= OCFS2_JOURNAL_TRIGGER_COUNT);
>>   -static struct ocfs2_triggers xb_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_xattr_block, xb_check),
>> -};
>> +    switch (type) {
>> +    case OCFS2_JTR_DI:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_dinode, i_check);
>> +        break;
>> +    case OCFS2_JTR_EB:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_extent_block, h_check);
>> +        break;
>> +    case OCFS2_JTR_RB:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_refcount_block, rf_check);
>> +        break;
>> +    case OCFS2_JTR_GD:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_group_desc, bg_check);
>> +        break;
>> +    case OCFS2_JTR_DB:
>> +        ot->ot_triggers.t_frozen = ocfs2_db_frozen_trigger;
>> +        break;
>> +    case OCFS2_JTR_XB:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_xattr_block, xb_check);
>> +        break;
>> +    case OCFS2_JTR_DQ:
>> +        ot->ot_triggers.t_frozen = ocfs2_dq_frozen_trigger;
>> +        break;
>> +    case OCFS2_JTR_DR:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_dx_root_block, dr_check);
>> +        break;
>> +    case OCFS2_JTR_DL:
>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>> +        ot->ot_offset = offsetof(struct ocfs2_dx_leaf, dl_check);
>> +        break;
>> +    case OCFS2_JTR_NONE:
>> +        /* To make compiler happy... */
>> +        return;
>> +    }
>>   -static struct ocfs2_triggers dq_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_dq_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -};
>> +    ot->ot_triggers.t_abort = ocfs2_abort_trigger;
>> +    ot->sb = sb;
>> +}
>>   -static struct ocfs2_triggers dr_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_dx_root_block, dr_check),
>> -};
>> +void ocfs2_initialize_journal_triggers(struct super_block *sb,
>> +                       struct ocfs2_triggers triggers[])
>> +{
>> +    enum ocfs2_journal_trigger_type type;
>>   -static struct ocfs2_triggers dl_triggers = {
>> -    .ot_triggers = {
>> -        .t_frozen = ocfs2_frozen_trigger,
>> -        .t_abort = ocfs2_abort_trigger,
>> -    },
>> -    .ot_offset    = offsetof(struct ocfs2_dx_leaf, dl_check),
>> -};
>> +    for (type = OCFS2_JTR_DI; type < OCFS2_JOURNAL_TRIGGER_COUNT; type++)
>> +        ocfs2_setup_csum_triggers(sb, type, &triggers[type]);
>> +}
>>     static int __ocfs2_journal_access(handle_t *handle,
>>                     struct ocfs2_caching_info *ci,
>> @@ -708,56 +693,91 @@ static int __ocfs2_journal_access(handle_t *handle,
>>   int ocfs2_journal_access_di(handle_t *handle, struct ocfs2_caching_info *ci,
>>                   struct buffer_head *bh, int type)
>>   {
>> -    return __ocfs2_journal_access(handle, ci, bh, &di_triggers, type);
>> +    struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
>> +
>> +    return __ocfs2_journal_access(handle, ci, bh,
>> +                      &osb->s_journal_triggers[OCFS2_JTR_DI],
>> +                      type);
> 
> Legacy code uses a static define '&di_triggers', new code does some pointer
> converting to fetch 'trigger'. I am not GCC expert, but in my view, it's better
> to add the 'inline' attribute for some (not every) ocfs2_journal_access_xx()
> function. e.g: ocfs2_journal_access_di().
> I checked some ocfs2_journal_access_di() callers, most of callers already
> have 'osb'. If GCC could inline this functions and remove the duplicated
> pointer conversions, there could save some cpu time.
> 

Compared with the old way, it may introduce a bit overhead.
But I don't want to change the APIs to pass 'osb' directly, which will
introduce lots of changes.
Since 'co_get_super' is a simple container_of operation, I think it's
acceptable.

Thanks,
Joseph
Heming Zhao June 3, 2024, 5:07 a.m. UTC | #3
On 6/3/24 11:43, Joseph Qi wrote:
> 
> 
> On 6/3/24 11:28 AM, Heming Zhao wrote:
>> Hello,
>>
>> This patch looks good to me. please see my comment below.
>>
>> On 6/2/24 19:20, Joseph Qi wrote:
>>> bdev->bd_super has been removed and commit 8887b94d9322 change the usage
>>> from bdev->bd_super to b_assoc_map->host->i_sb. Since ocfs2 hasn't set
>>> bh->b_assoc_map, it will trigger NULL pointer dereference when calling
>>> into ocfs2_abort_trigger().
>>>
>>> Actually this was pointed out in history, see commit 74e364ad1b13. But
>>> I've made a mistake when reviewing commit 8887b94d9322 and then
>>> re-introduce this regression.
>>>
>>> Since we cannot revive bdev in buffer head, so fix this issue by
>>> initializing all types of ocfs2 triggers when fill super, and then get
>>> the specific ocfs2 trigger from ocfs2_caching_info when access journal.
>>>
>>> Fixes: 8887b94d9322 ("ocfs2: stop using bdev->bd_super for journal error logging")
>>> Cc: stable@vger.kernel.org # 6.6+
>>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Reviewed-by: Heming Zhao <heming.zhao@suse.com>

>>> ---
>>>    fs/ocfs2/journal.c | 182 +++++++++++++++++++++++++--------------------
>>>    fs/ocfs2/ocfs2.h   |  27 +++++++
>>>    fs/ocfs2/super.c   |   4 +-
>>>    3 files changed, 131 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>>> index 27c7683c7d3f..86807086b2df 100644
>>> --- a/fs/ocfs2/journal.c
>>> +++ b/fs/ocfs2/journal.c
>>> @@ -479,12 +479,6 @@ int ocfs2_allocate_extend_trans(handle_t *handle, int thresh)
>>>        return status;
>>>    }
>>>    -
>>> -struct ocfs2_triggers {
>>> -    struct jbd2_buffer_trigger_type    ot_triggers;
>>> -    int                ot_offset;
>>> -};
>>> -
>>>    static inline struct ocfs2_triggers *to_ocfs2_trigger(struct jbd2_buffer_trigger_type *triggers)
>>>    {
>>>        return container_of(triggers, struct ocfs2_triggers, ot_triggers);
>>> @@ -548,85 +542,76 @@ static void ocfs2_db_frozen_trigger(struct jbd2_buffer_trigger_type *triggers,
>>>    static void ocfs2_abort_trigger(struct jbd2_buffer_trigger_type *triggers,
>>>                    struct buffer_head *bh)
>>>    {
>>> +    struct ocfs2_triggers *ot = to_ocfs2_trigger(triggers);
>>> +
>>>        mlog(ML_ERROR,
>>>             "ocfs2_abort_trigger called by JBD2.  bh = 0x%lx, "
>>>             "bh->b_blocknr = %llu\n",
>>>             (unsigned long)bh,
>>>             (unsigned long long)bh->b_blocknr);
>>>    -    ocfs2_error(bh->b_assoc_map->host->i_sb,
>>> +    ocfs2_error(ot->sb,
>>>                "JBD2 has aborted our journal, ocfs2 cannot continue\n");
>>>    }
>>>    -static struct ocfs2_triggers di_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_dinode, i_check),
>>> -};
>>> -
>>> -static struct ocfs2_triggers eb_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_extent_block, h_check),
>>> -};
>>> -
>>> -static struct ocfs2_triggers rb_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_refcount_block, rf_check),
>>> -};
>>> -
>>> -static struct ocfs2_triggers gd_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_group_desc, bg_check),
>>> -};
>>> -
>>> -static struct ocfs2_triggers db_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_db_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -};
>>> +static void ocfs2_setup_csum_triggers(struct super_block *sb,
>>> +                      enum ocfs2_journal_trigger_type type,
>>> +                      struct ocfs2_triggers *ot)
>>> +{
>>> +    BUG_ON(type >= OCFS2_JOURNAL_TRIGGER_COUNT);
>>>    -static struct ocfs2_triggers xb_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_xattr_block, xb_check),
>>> -};
>>> +    switch (type) {
>>> +    case OCFS2_JTR_DI:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_dinode, i_check);
>>> +        break;
>>> +    case OCFS2_JTR_EB:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_extent_block, h_check);
>>> +        break;
>>> +    case OCFS2_JTR_RB:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_refcount_block, rf_check);
>>> +        break;
>>> +    case OCFS2_JTR_GD:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_group_desc, bg_check);
>>> +        break;
>>> +    case OCFS2_JTR_DB:
>>> +        ot->ot_triggers.t_frozen = ocfs2_db_frozen_trigger;
>>> +        break;
>>> +    case OCFS2_JTR_XB:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_xattr_block, xb_check);
>>> +        break;
>>> +    case OCFS2_JTR_DQ:
>>> +        ot->ot_triggers.t_frozen = ocfs2_dq_frozen_trigger;
>>> +        break;
>>> +    case OCFS2_JTR_DR:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_dx_root_block, dr_check);
>>> +        break;
>>> +    case OCFS2_JTR_DL:
>>> +        ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
>>> +        ot->ot_offset = offsetof(struct ocfs2_dx_leaf, dl_check);
>>> +        break;
>>> +    case OCFS2_JTR_NONE:
>>> +        /* To make compiler happy... */
>>> +        return;
>>> +    }
>>>    -static struct ocfs2_triggers dq_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_dq_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -};
>>> +    ot->ot_triggers.t_abort = ocfs2_abort_trigger;
>>> +    ot->sb = sb;
>>> +}
>>>    -static struct ocfs2_triggers dr_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_dx_root_block, dr_check),
>>> -};
>>> +void ocfs2_initialize_journal_triggers(struct super_block *sb,
>>> +                       struct ocfs2_triggers triggers[])
>>> +{
>>> +    enum ocfs2_journal_trigger_type type;
>>>    -static struct ocfs2_triggers dl_triggers = {
>>> -    .ot_triggers = {
>>> -        .t_frozen = ocfs2_frozen_trigger,
>>> -        .t_abort = ocfs2_abort_trigger,
>>> -    },
>>> -    .ot_offset    = offsetof(struct ocfs2_dx_leaf, dl_check),
>>> -};
>>> +    for (type = OCFS2_JTR_DI; type < OCFS2_JOURNAL_TRIGGER_COUNT; type++)
>>> +        ocfs2_setup_csum_triggers(sb, type, &triggers[type]);
>>> +}
>>>      static int __ocfs2_journal_access(handle_t *handle,
>>>                      struct ocfs2_caching_info *ci,
>>> @@ -708,56 +693,91 @@ static int __ocfs2_journal_access(handle_t *handle,
>>>    int ocfs2_journal_access_di(handle_t *handle, struct ocfs2_caching_info *ci,
>>>                    struct buffer_head *bh, int type)
>>>    {
>>> -    return __ocfs2_journal_access(handle, ci, bh, &di_triggers, type);
>>> +    struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
>>> +
>>> +    return __ocfs2_journal_access(handle, ci, bh,
>>> +                      &osb->s_journal_triggers[OCFS2_JTR_DI],
>>> +                      type);
>>
>> Legacy code uses a static define '&di_triggers', new code does some pointer
>> converting to fetch 'trigger'. I am not GCC expert, but in my view, it's better
>> to add the 'inline' attribute for some (not every) ocfs2_journal_access_xx()
>> function. e.g: ocfs2_journal_access_di().
>> I checked some ocfs2_journal_access_di() callers, most of callers already
>> have 'osb'. If GCC could inline this functions and remove the duplicated
>> pointer conversions, there could save some cpu time.
>>
> 
> Compared with the old way, it may introduce a bit overhead.
> But I don't want to change the APIs to pass 'osb' directly, which will
> introduce lots of changes.
> Since 'co_get_super' is a simple container_of operation, I think it's
> acceptable.
> 
> Thanks,
> Joseph
diff mbox series

Patch

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 27c7683c7d3f..86807086b2df 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -479,12 +479,6 @@  int ocfs2_allocate_extend_trans(handle_t *handle, int thresh)
 	return status;
 }
 
-
-struct ocfs2_triggers {
-	struct jbd2_buffer_trigger_type	ot_triggers;
-	int				ot_offset;
-};
-
 static inline struct ocfs2_triggers *to_ocfs2_trigger(struct jbd2_buffer_trigger_type *triggers)
 {
 	return container_of(triggers, struct ocfs2_triggers, ot_triggers);
@@ -548,85 +542,76 @@  static void ocfs2_db_frozen_trigger(struct jbd2_buffer_trigger_type *triggers,
 static void ocfs2_abort_trigger(struct jbd2_buffer_trigger_type *triggers,
 				struct buffer_head *bh)
 {
+	struct ocfs2_triggers *ot = to_ocfs2_trigger(triggers);
+
 	mlog(ML_ERROR,
 	     "ocfs2_abort_trigger called by JBD2.  bh = 0x%lx, "
 	     "bh->b_blocknr = %llu\n",
 	     (unsigned long)bh,
 	     (unsigned long long)bh->b_blocknr);
 
-	ocfs2_error(bh->b_assoc_map->host->i_sb,
+	ocfs2_error(ot->sb,
 		    "JBD2 has aborted our journal, ocfs2 cannot continue\n");
 }
 
-static struct ocfs2_triggers di_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_dinode, i_check),
-};
-
-static struct ocfs2_triggers eb_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_extent_block, h_check),
-};
-
-static struct ocfs2_triggers rb_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_refcount_block, rf_check),
-};
-
-static struct ocfs2_triggers gd_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_group_desc, bg_check),
-};
-
-static struct ocfs2_triggers db_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_db_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-};
+static void ocfs2_setup_csum_triggers(struct super_block *sb,
+				      enum ocfs2_journal_trigger_type type,
+				      struct ocfs2_triggers *ot)
+{
+	BUG_ON(type >= OCFS2_JOURNAL_TRIGGER_COUNT);
 
-static struct ocfs2_triggers xb_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_xattr_block, xb_check),
-};
+	switch (type) {
+	case OCFS2_JTR_DI:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_dinode, i_check);
+		break;
+	case OCFS2_JTR_EB:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_extent_block, h_check);
+		break;
+	case OCFS2_JTR_RB:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_refcount_block, rf_check);
+		break;
+	case OCFS2_JTR_GD:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_group_desc, bg_check);
+		break;
+	case OCFS2_JTR_DB:
+		ot->ot_triggers.t_frozen = ocfs2_db_frozen_trigger;
+		break;
+	case OCFS2_JTR_XB:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_xattr_block, xb_check);
+		break;
+	case OCFS2_JTR_DQ:
+		ot->ot_triggers.t_frozen = ocfs2_dq_frozen_trigger;
+		break;
+	case OCFS2_JTR_DR:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_dx_root_block, dr_check);
+		break;
+	case OCFS2_JTR_DL:
+		ot->ot_triggers.t_frozen = ocfs2_frozen_trigger;
+		ot->ot_offset = offsetof(struct ocfs2_dx_leaf, dl_check);
+		break;
+	case OCFS2_JTR_NONE:
+		/* To make compiler happy... */
+		return;
+	}
 
-static struct ocfs2_triggers dq_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_dq_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-};
+	ot->ot_triggers.t_abort = ocfs2_abort_trigger;
+	ot->sb = sb;
+}
 
-static struct ocfs2_triggers dr_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_dx_root_block, dr_check),
-};
+void ocfs2_initialize_journal_triggers(struct super_block *sb,
+				       struct ocfs2_triggers triggers[])
+{
+	enum ocfs2_journal_trigger_type type;
 
-static struct ocfs2_triggers dl_triggers = {
-	.ot_triggers = {
-		.t_frozen = ocfs2_frozen_trigger,
-		.t_abort = ocfs2_abort_trigger,
-	},
-	.ot_offset	= offsetof(struct ocfs2_dx_leaf, dl_check),
-};
+	for (type = OCFS2_JTR_DI; type < OCFS2_JOURNAL_TRIGGER_COUNT; type++)
+		ocfs2_setup_csum_triggers(sb, type, &triggers[type]);
+}
 
 static int __ocfs2_journal_access(handle_t *handle,
 				  struct ocfs2_caching_info *ci,
@@ -708,56 +693,91 @@  static int __ocfs2_journal_access(handle_t *handle,
 int ocfs2_journal_access_di(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &di_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				      &osb->s_journal_triggers[OCFS2_JTR_DI],
+				      type);
 }
 
 int ocfs2_journal_access_eb(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &eb_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				      &osb->s_journal_triggers[OCFS2_JTR_EB],
+				      type);
 }
 
 int ocfs2_journal_access_rb(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &rb_triggers,
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				      &osb->s_journal_triggers[OCFS2_JTR_RB],
 				      type);
 }
 
 int ocfs2_journal_access_gd(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &gd_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				     &osb->s_journal_triggers[OCFS2_JTR_GD],
+				     type);
 }
 
 int ocfs2_journal_access_db(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &db_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				     &osb->s_journal_triggers[OCFS2_JTR_DB],
+				     type);
 }
 
 int ocfs2_journal_access_xb(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &xb_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				     &osb->s_journal_triggers[OCFS2_JTR_XB],
+				     type);
 }
 
 int ocfs2_journal_access_dq(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &dq_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				     &osb->s_journal_triggers[OCFS2_JTR_DQ],
+				     type);
 }
 
 int ocfs2_journal_access_dr(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &dr_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				     &osb->s_journal_triggers[OCFS2_JTR_DR],
+				     type);
 }
 
 int ocfs2_journal_access_dl(handle_t *handle, struct ocfs2_caching_info *ci,
 			    struct buffer_head *bh, int type)
 {
-	return __ocfs2_journal_access(handle, ci, bh, &dl_triggers, type);
+	struct ocfs2_super *osb = OCFS2_SB(ocfs2_metadata_cache_get_super(ci));
+
+	return __ocfs2_journal_access(handle, ci, bh,
+				     &osb->s_journal_triggers[OCFS2_JTR_DL],
+				     type);
 }
 
 int ocfs2_journal_access(handle_t *handle, struct ocfs2_caching_info *ci,
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index a503c553bab2..8fe826143d7b 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -284,6 +284,30 @@  enum ocfs2_mount_options
 #define OCFS2_OSB_ERROR_FS	0x0004
 #define OCFS2_DEFAULT_ATIME_QUANTUM	60
 
+struct ocfs2_triggers {
+	struct jbd2_buffer_trigger_type	ot_triggers;
+	int				ot_offset;
+	struct super_block		*sb;
+};
+
+enum ocfs2_journal_trigger_type {
+	OCFS2_JTR_DI,
+	OCFS2_JTR_EB,
+	OCFS2_JTR_RB,
+	OCFS2_JTR_GD,
+	OCFS2_JTR_DB,
+	OCFS2_JTR_XB,
+	OCFS2_JTR_DQ,
+	OCFS2_JTR_DR,
+	OCFS2_JTR_DL,
+	OCFS2_JTR_NONE  /* This must be the last entry */
+};
+
+#define OCFS2_JOURNAL_TRIGGER_COUNT OCFS2_JTR_NONE
+
+void ocfs2_initialize_journal_triggers(struct super_block *sb,
+				       struct ocfs2_triggers triggers[]);
+
 struct ocfs2_journal;
 struct ocfs2_slot_info;
 struct ocfs2_recovery_map;
@@ -351,6 +375,9 @@  struct ocfs2_super
 	struct ocfs2_journal *journal;
 	unsigned long osb_commit_interval;
 
+	/* Journal triggers for checksum */
+	struct ocfs2_triggers s_journal_triggers[OCFS2_JOURNAL_TRIGGER_COUNT];
+
 	struct delayed_work		la_enable_wq;
 
 	/*
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 8aabaed2c1cb..afee70125ae3 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1075,9 +1075,11 @@  static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
 	debugfs_create_file("fs_state", S_IFREG|S_IRUSR, osb->osb_debug_root,
 			    osb, &ocfs2_osb_debug_fops);
 
-	if (ocfs2_meta_ecc(osb))
+	if (ocfs2_meta_ecc(osb)) {
+		ocfs2_initialize_journal_triggers(sb, osb->s_journal_triggers);
 		ocfs2_blockcheck_stats_debugfs_install( &osb->osb_ecc_stats,
 							osb->osb_debug_root);
+	}
 
 	status = ocfs2_mount_volume(sb);
 	if (status < 0)