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 *))
{
...
}
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
