On 2015-03-10 16:52, Ryusuke Konishi wrote:
> On Tue, 24 Feb 2015 20:01:36 +0100, Andreas Rohner wrote:
>> This patch refactors nilfs_sufile_updatev() to take an array of
>> arbitrary data structures instead of an array of segment numbers as
>> input parameter. With this change it is reusable for cases, where
>> it is necessary to pass extra data to the update function. The only
>> requirement for the data structures passed as input is, that they
>> contain the segment number within the structure. By passing the
>> offset to the segment number as another input parameter,
>> nilfs_sufile_updatev() can be oblivious to the actual type of the
>> input structures in the array.
>>
>> Signed-off-by: Andreas Rohner <andreas.rohner@xxxxxxx>
>> ---
>> fs/nilfs2/sufile.c | 79 ++++++++++++++++++++++++++++++++----------------------
>> fs/nilfs2/sufile.h | 39 ++++++++++++++-------------
>> 2 files changed, 68 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
>> index 2a869c3..1e8cac6 100644
>> --- a/fs/nilfs2/sufile.c
>> +++ b/fs/nilfs2/sufile.c
>> @@ -138,14 +138,18 @@ unsigned long nilfs_sufile_get_ncleansegs(struct inode *sufile)
>> /**
>> * nilfs_sufile_updatev - modify multiple segment usages at a time
>> * @sufile: inode of segment usage file
>> - * @segnumv: array of segment numbers
>> - * @nsegs: size of @segnumv array
>> + * @datav: array of segment numbers
>> + * @datasz: size of elements in @datav
>> + * @segoff: offset to segnum within the elements of @datav
>> + * @ndata: size of @datav array
>> * @create: creation flag
>> * @ndone: place to store number of modified segments on @segnumv
>> * @dofunc: primitive operation for the update
>> *
>> * Description: nilfs_sufile_updatev() repeatedly calls @dofunc
>> - * against the given array of segments. The @dofunc is called with
>> + * against the given array of data elements. Every data element has
>> + * to contain a valid segment number and @segoff should be the offset
>> + * to that within the data structure. The @dofunc is called with
>> * buffers of a header block and the sufile block in which the target
>> * segment usage entry is contained. If @ndone is given, the number
>> * of successfully modified segments from the head is stored in the
>> @@ -163,50 +167,55 @@ unsigned long nilfs_sufile_get_ncleansegs(struct inode *sufile)
>> *
>> * %-EINVAL - Invalid segment usage number
>> */
>> -int nilfs_sufile_updatev(struct inode *sufile, __u64 *segnumv, size_t nsegs,
>> - int create, size_t *ndone,
>> - void (*dofunc)(struct inode *, __u64,
>> +int nilfs_sufile_updatev(struct inode *sufile, void *datav, size_t datasz,
>> + size_t segoff, size_t ndata, int create,
>> + size_t *ndone,
>> + void (*dofunc)(struct inode *, void *,
>> struct buffer_head *,
>> struct buffer_head *))
>
> Using offset byte of the data like segoff is nasty.
>
> Please consider defining a template structure and its variation:
>
> struct nilfs_sufile_update_data {
> __u64 segnum;
> /* Optional data comes after segnum */
> };
>
> /**
> * struct nilfs_sufile_update_count - data type of nilfs_sufile_do_xxx
> * @segnum: segment number
> * @nadd: additional value to a counter
> * Description: This structure derives from nilfs_sufile_update_data
> * struct.
> */
> struct nilfs_sufile_update_count {
> __u64 segnum;
> __u64 nadd;
> };
>
> int nilfs_sufile_updatev(struct inode *sufile,
> struct nilfs_sufile_update_data *datav,
> size_t datasz,
> size_t ndata, int create, size_t *ndone,
> void (*dofunc)(struct inode *,
> struct nilfs_sufile_update_data *,
> struct buffer_head *,
> struct buffer_head *))
> {
> ...
> }
I agree this is a much better solution. I'll change it.
Regards,
Andreas Rohner
> If you need define segnum in the middle of structure, you can use
> container_of():
>
> Example:
>
> struct nilfs_sufile_update_xxx {
> __u32 item_a;
> __u32 item_b;
> struct nilfs_sufile_update_data u_data;
> };
>
> static inline struct nilfs_sufile_update_xxx *
> NILFS_SU_UPDATE_XXX(struct nilfs_sufile_update_data *data)
> {
> return container_of(data, struct nilfs_sufile_update_xxx, u_data);
> }
>
> void nilfs_sufile_do_xxx(...)
> {
> struct nilfs_sufile_update_xxx *xxx;
>
> xxx = NILFS_SU_UPDATA_XXX(data);
> ...
> }
>
> I believe the former technique is enough in your case. (you can
> suppose that segnum is always the first member of data, right ?).
>
>
>> {
>> struct buffer_head *header_bh, *bh;
>> unsigned long blkoff, prev_blkoff;
>> __u64 *seg;
>> - size_t nerr = 0, n = 0;
>> + void *data, *dataend = datav + ndata * datasz;
>> + size_t n = 0;
>> int ret = 0;
>>
>> - if (unlikely(nsegs == 0))
>> + if (unlikely(ndata == 0))
>> goto out;
>>
>> - down_write(&NILFS_MDT(sufile)->mi_sem);
>> - for (seg = segnumv; seg < segnumv + nsegs; seg++) {
>> +
>> + for (data = datav; data < dataend; data += datasz) {
>> + seg = data + segoff;
>> if (unlikely(*seg >= nilfs_sufile_get_nsegments(sufile))) {
>> printk(KERN_WARNING
>> "%s: invalid segment number: %llu\n", __func__,
>> (unsigned long long)*seg);
>> - nerr++;
>> + ret = -EINVAL;
>> + goto out;
>> }
>> }
>> - if (nerr > 0) {
>> - ret = -EINVAL;
>> - goto out_sem;
>> - }
>>
>> + down_write(&NILFS_MDT(sufile)->mi_sem);
>> ret = nilfs_sufile_get_header_block(sufile, &header_bh);
>> if (ret < 0)
>> goto out_sem;
>>
>> - seg = segnumv;
>> + data = datav;
>> + seg = data + segoff;
>> blkoff = nilfs_sufile_get_blkoff(sufile, *seg);
>> ret = nilfs_mdt_get_block(sufile, blkoff, create, NULL, &bh);
>> if (ret < 0)
>> goto out_header;
>>
>> for (;;) {
>> - dofunc(sufile, *seg, header_bh, bh);
>> + dofunc(sufile, data, header_bh, bh);
>>
>> - if (++seg >= segnumv + nsegs)
>> + ++n;
>> + data += datasz;
>> + if (data >= dataend)
>> break;
>> + seg = data + segoff;
>> prev_blkoff = blkoff;
>> blkoff = nilfs_sufile_get_blkoff(sufile, *seg);
>> if (blkoff == prev_blkoff)
>> @@ -220,28 +229,30 @@ int nilfs_sufile_updatev(struct inode *sufile, __u64 *segnumv, size_t nsegs,
>> }
>> brelse(bh);
>>
>> - out_header:
>> - n = seg - segnumv;
>> +out_header:
>> brelse(header_bh);
>> - out_sem:
>> +out_sem:
>> up_write(&NILFS_MDT(sufile)->mi_sem);
>> - out:
>> +out:
>> if (ndone)
>> *ndone = n;
>> return ret;
>> }
>>
>> -int nilfs_sufile_update(struct inode *sufile, __u64 segnum, int create,
>> - void (*dofunc)(struct inode *, __u64,
>> +int nilfs_sufile_update(struct inode *sufile, void *data, size_t segoff,
>> + int create,
>> + void (*dofunc)(struct inode *, void *,
>> struct buffer_head *,
>> struct buffer_head *))
>
> ditto.
>
>> {
>> struct buffer_head *header_bh, *bh;
>> + __u64 *seg;
>> int ret;
>>
>> - if (unlikely(segnum >= nilfs_sufile_get_nsegments(sufile))) {
>> + seg = data + segoff;
>> + if (unlikely(*seg >= nilfs_sufile_get_nsegments(sufile))) {
>> printk(KERN_WARNING "%s: invalid segment number: %llu\n",
>> - __func__, (unsigned long long)segnum);
>> + __func__, (unsigned long long)*seg);
>> return -EINVAL;
>> }
>
> You can remove these nasty changes.
>
>> down_write(&NILFS_MDT(sufile)->mi_sem);
>> @@ -250,9 +261,9 @@ int nilfs_sufile_update(struct inode *sufile, __u64 segnum, int create,
>> if (ret < 0)
>> goto out_sem;
>>
>> - ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, create, &bh);
>> + ret = nilfs_sufile_get_segment_usage_block(sufile, *seg, create, &bh);
>
> ditto.
>
>> if (!ret) {
>> - dofunc(sufile, segnum, header_bh, bh);
>> + dofunc(sufile, data, header_bh, bh);
>> brelse(bh);
>> }
>> brelse(header_bh);
>> @@ -406,12 +417,13 @@ int nilfs_sufile_alloc(struct inode *sufile, __u64 *segnump)
>> return ret;
>> }
>>
>> -void nilfs_sufile_do_cancel_free(struct inode *sufile, __u64 segnum,
>> +void nilfs_sufile_do_cancel_free(struct inode *sufile, __u64 *data,
>> struct buffer_head *header_bh,
>> struct buffer_head *su_bh)
>> {
>> struct nilfs_segment_usage *su;
>> void *kaddr;
>> + __u64 segnum = *data;
>>
>> kaddr = kmap_atomic(su_bh->b_page);
>> su = nilfs_sufile_block_get_segment_usage(sufile, segnum, su_bh, kaddr);
>> @@ -431,13 +443,14 @@ void nilfs_sufile_do_cancel_free(struct inode *sufile, __u64 segnum,
>> nilfs_mdt_mark_dirty(sufile);
>> }
>>
>> -void nilfs_sufile_do_scrap(struct inode *sufile, __u64 segnum,
>> +void nilfs_sufile_do_scrap(struct inode *sufile, __u64 *data,
>> struct buffer_head *header_bh,
>> struct buffer_head *su_bh)
>> {
>> struct nilfs_segment_usage *su;
>> void *kaddr;
>> int clean, dirty;
>> + __u64 segnum = *data;
>
> This can be converted to as follows:
>
> __u64 segnum = data->segnum;
>
>>
>> kaddr = kmap_atomic(su_bh->b_page);
>> su = nilfs_sufile_block_get_segment_usage(sufile, segnum, su_bh, kaddr);
>> @@ -462,13 +475,14 @@ void nilfs_sufile_do_scrap(struct inode *sufile, __u64 segnum,
>> nilfs_mdt_mark_dirty(sufile);
>> }
>>
>> -void nilfs_sufile_do_free(struct inode *sufile, __u64 segnum,
>> +void nilfs_sufile_do_free(struct inode *sufile, __u64 *data,
>> struct buffer_head *header_bh,
>> struct buffer_head *su_bh)
>> {
>> struct nilfs_segment_usage *su;
>> void *kaddr;
>> int sudirty;
>> + __u64 segnum = *data;
>>
>> kaddr = kmap_atomic(su_bh->b_page);
>> su = nilfs_sufile_block_get_segment_usage(sufile, segnum, su_bh, kaddr);
>> @@ -596,13 +610,14 @@ int nilfs_sufile_get_stat(struct inode *sufile, struct nilfs_sustat *sustat)
>> return ret;
>> }
>>
>> -void nilfs_sufile_do_set_error(struct inode *sufile, __u64 segnum,
>> +void nilfs_sufile_do_set_error(struct inode *sufile, __u64 *data,
>> struct buffer_head *header_bh,
>> struct buffer_head *su_bh)
>> {
>> struct nilfs_segment_usage *su;
>> void *kaddr;
>> int suclean;
>> + __u64 segnum = *data;
>>
>> kaddr = kmap_atomic(su_bh->b_page);
>> su = nilfs_sufile_block_get_segment_usage(sufile, segnum, su_bh, kaddr);
>> diff --git a/fs/nilfs2/sufile.h b/fs/nilfs2/sufile.h
>> index b8afd72..2df6c71 100644
>> --- a/fs/nilfs2/sufile.h
>> +++ b/fs/nilfs2/sufile.h
>> @@ -46,21 +46,21 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *, __u64, void *, unsigned,
>> size_t);
>> ssize_t nilfs_sufile_set_suinfo(struct inode *, void *, unsigned , size_t);
>>
>> -int nilfs_sufile_updatev(struct inode *, __u64 *, size_t, int, size_t *,
>> - void (*dofunc)(struct inode *, __u64,
>> - struct buffer_head *,
>> - struct buffer_head *));
>> -int nilfs_sufile_update(struct inode *, __u64, int,
>> - void (*dofunc)(struct inode *, __u64,
>> +int nilfs_sufile_updatev(struct inode *, void *, size_t, size_t, size_t, int,
>> + size_t *, void (*dofunc)(struct inode *, void *,
>> + struct buffer_head *,
>> + struct buffer_head *));
>> +int nilfs_sufile_update(struct inode *, void *, size_t, int,
>> + void (*dofunc)(struct inode *, void *,
>> struct buffer_head *,
>> struct buffer_head *));
>
>> -void nilfs_sufile_do_scrap(struct inode *, __u64, struct buffer_head *,
>> +void nilfs_sufile_do_scrap(struct inode *, __u64 *, struct buffer_head *,
>> struct buffer_head *);
>> -void nilfs_sufile_do_free(struct inode *, __u64, struct buffer_head *,
>> +void nilfs_sufile_do_free(struct inode *, __u64 *, struct buffer_head *,
>> struct buffer_head *);
>> -void nilfs_sufile_do_cancel_free(struct inode *, __u64, struct buffer_head *,
>> +void nilfs_sufile_do_cancel_free(struct inode *, __u64 *, struct buffer_head *,
>> struct buffer_head *);
>> -void nilfs_sufile_do_set_error(struct inode *, __u64, struct buffer_head *,
>> +void nilfs_sufile_do_set_error(struct inode *, __u64 *, struct buffer_head *,
>> struct buffer_head *);
>
> Please, use "struct nilfs_sufile_update_data *" type for the second
> argument of these declaration.
>
>>
>> int nilfs_sufile_resize(struct inode *sufile, __u64 newnsegs);
>> @@ -75,7 +75,8 @@ int nilfs_sufile_trim_fs(struct inode *sufile, struct fstrim_range *range);
>> */
>> static inline int nilfs_sufile_scrap(struct inode *sufile, __u64 segnum)
>> {
>> - return nilfs_sufile_update(sufile, segnum, 1, nilfs_sufile_do_scrap);
>> + return nilfs_sufile_update(sufile, &segnum, 0, 1,
>> + (void *)nilfs_sufile_do_scrap);
>> }
>
> Then you can avoid this nasty (void *) cast to the callback function.
>
>>
>> /**
>> @@ -85,7 +86,8 @@ static inline int nilfs_sufile_scrap(struct inode *sufile, __u64 segnum)
>> */
>> static inline int nilfs_sufile_free(struct inode *sufile, __u64 segnum)
>> {
>> - return nilfs_sufile_update(sufile, segnum, 0, nilfs_sufile_do_free);
>> + return nilfs_sufile_update(sufile, &segnum, 0, 0,
>> + (void *)nilfs_sufile_do_free);
>> }
>
> ditto
>
>> /**
>> @@ -98,8 +100,8 @@ static inline int nilfs_sufile_free(struct inode *sufile, __u64 segnum)
>> static inline int nilfs_sufile_freev(struct inode *sufile, __u64 *segnumv,
>> size_t nsegs, size_t *ndone)
>> {
>> - return nilfs_sufile_updatev(sufile, segnumv, nsegs, 0, ndone,
>> - nilfs_sufile_do_free);
>> + return nilfs_sufile_updatev(sufile, segnumv, sizeof(__u64), 0, nsegs,
>> + 0, ndone, (void *)nilfs_sufile_do_free);
>> }
>
> ditto
>
>> /**
>> @@ -116,8 +118,9 @@ static inline int nilfs_sufile_cancel_freev(struct inode *sufile,
>> __u64 *segnumv, size_t nsegs,
>> size_t *ndone)
>> {
>> - return nilfs_sufile_updatev(sufile, segnumv, nsegs, 0, ndone,
>> - nilfs_sufile_do_cancel_free);
>> + return nilfs_sufile_updatev(sufile, segnumv, sizeof(__u64), 0, nsegs,
>> + 0, ndone,
>> + (void *)nilfs_sufile_do_cancel_free);
>> }
>
> ditto
>
>> /**
>> @@ -139,8 +142,8 @@ static inline int nilfs_sufile_cancel_freev(struct inode *sufile,
>> */
>> static inline int nilfs_sufile_set_error(struct inode *sufile, __u64 segnum)
>> {
>> - return nilfs_sufile_update(sufile, segnum, 0,
>> - nilfs_sufile_do_set_error);
>> + return nilfs_sufile_update(sufile, &segnum, 0, 0,
>> + (void *)nilfs_sufile_do_set_error);
>> }
>>
>> #endif /* _NILFS_SUFILE_H */
>
> ditto
>
>
> Regards,
> Ryusuke Konishi
>
>> --
>> 2.3.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
