| Message ID | 20170725205138.28376-5-jeffm@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25.07.2017 23:51, jeffm@suse.com wrote: > From: Jeff Mahoney <jeffm@suse.com> > > --- > backref.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/backref.c b/backref.c > index ac1b506..be3376a 100644 > --- a/backref.c > +++ b/backref.c > @@ -130,6 +130,11 @@ struct __prelim_ref { > u64 wanted_disk_byte; > }; > > +static struct __prelim_ref *list_first_pref(struct list_head *head) > +{ > + return list_first_entry(head, struct __prelim_ref, list); > +} > + I think this just adds one more level of abstraction with no real benefit whatsoever. Why not drop the patch entirely. > struct pref_state { > struct list_head pending; > }; > @@ -804,8 +809,7 @@ static int find_parent_nodes(struct btrfs_trans_handle *trans, > __merge_refs(&prefstate, 2); > > while (!list_empty(&prefstate.pending)) { > - ref = list_first_entry(&prefstate.pending, > - struct __prelim_ref, list); > + ref = list_first_pref(&prefstate.pending); > WARN_ON(ref->count < 0); > if (roots && ref->count && ref->root_id && ref->parent == 0) { > /* no parent == root of tree */ > @@ -857,8 +861,7 @@ static int find_parent_nodes(struct btrfs_trans_handle *trans, > out: > btrfs_free_path(path); > while (!list_empty(&prefstate.pending)) { > - ref = list_first_entry(&prefstate.pending, > - struct __prelim_ref, list); > + ref = list_first_pref(&prefstate.pending); > list_del(&ref->list); > kfree(ref); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/26/17 3:08 AM, Nikolay Borisov wrote: > > > On 25.07.2017 23:51, jeffm@suse.com wrote: >> From: Jeff Mahoney <jeffm@suse.com> >> >> --- >> backref.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/backref.c b/backref.c >> index ac1b506..be3376a 100644 >> --- a/backref.c >> +++ b/backref.c >> @@ -130,6 +130,11 @@ struct __prelim_ref { >> u64 wanted_disk_byte; >> }; >> >> +static struct __prelim_ref *list_first_pref(struct list_head *head) >> +{ >> + return list_first_entry(head, struct __prelim_ref, list); >> +} >> + > > I think this just adds one more level of abstraction with no real > benefit whatsoever. Why not drop the patch entirely. Ack. I thought it might be more readable but it ends up taking the same number of characters. -Jeff
On 7/26/17 9:22 AM, Jeff Mahoney wrote: > On 7/26/17 3:08 AM, Nikolay Borisov wrote: >> >> >> On 25.07.2017 23:51, jeffm@suse.com wrote: >>> From: Jeff Mahoney <jeffm@suse.com> >>> >>> --- >>> backref.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/backref.c b/backref.c >>> index ac1b506..be3376a 100644 >>> --- a/backref.c >>> +++ b/backref.c >>> @@ -130,6 +130,11 @@ struct __prelim_ref { >>> u64 wanted_disk_byte; >>> }; >>> >>> +static struct __prelim_ref *list_first_pref(struct list_head *head) >>> +{ >>> + return list_first_entry(head, struct __prelim_ref, list); >>> +} >>> + >> >> I think this just adds one more level of abstraction with no real >> benefit whatsoever. Why not drop the patch entirely. > > Ack. I thought it might be more readable but it ends up taking the same > number of characters. Actually, no, it doesn't. That's only true if using 'head' as the list head as in the helper. It ends up being ref = list_first_pref(&prefstate->pending_missing_keys); vs ref = list_first_entry(&prefstate->pending_missing_keys, struct __prelim_ref, list); and I have to say I prefer reading the former. -Jeff
diff --git a/backref.c b/backref.c index ac1b506..be3376a 100644 --- a/backref.c +++ b/backref.c @@ -130,6 +130,11 @@ struct __prelim_ref { u64 wanted_disk_byte; }; +static struct __prelim_ref *list_first_pref(struct list_head *head) +{ + return list_first_entry(head, struct __prelim_ref, list); +} + struct pref_state { struct list_head pending; }; @@ -804,8 +809,7 @@ static int find_parent_nodes(struct btrfs_trans_handle *trans, __merge_refs(&prefstate, 2); while (!list_empty(&prefstate.pending)) { - ref = list_first_entry(&prefstate.pending, - struct __prelim_ref, list); + ref = list_first_pref(&prefstate.pending); WARN_ON(ref->count < 0); if (roots && ref->count && ref->root_id && ref->parent == 0) { /* no parent == root of tree */ @@ -857,8 +861,7 @@ static int find_parent_nodes(struct btrfs_trans_handle *trans, out: btrfs_free_path(path); while (!list_empty(&prefstate.pending)) { - ref = list_first_entry(&prefstate.pending, - struct __prelim_ref, list); + ref = list_first_pref(&prefstate.pending); list_del(&ref->list); kfree(ref); }
From: Jeff Mahoney <jeffm@suse.com> --- backref.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)