On 2014-01-27 20:07, Ryusuke Konishi wrote:
> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>> This patch introduces the nilfs_sufile_set_suinfo function, which
>> expects an array of nilfs_suinfo_update structures and updates the
>> segment usage information accordingly.
>>
>> This is basically a helper function for the newly introduced
>> NILFS_IOCTL_SET_SUINFO ioctl.
>>
>> Signed-off-by: Andreas Rohner <[email protected]>
>> ---
>> fs/nilfs2/sufile.c | 129
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nilfs2/sufile.h | 1 +
>> 2 files changed, 130 insertions(+)
>>
>> diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
>> index 3127e9f..8922523 100644
>> --- a/fs/nilfs2/sufile.c
>> +++ b/fs/nilfs2/sufile.c
>> @@ -870,6 +870,135 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile,
>> __u64 segnum, void *buf,
>> }
>>
>> /**
>> + * nilfs_sufile_set_suinfo - sets segment usage info
>> + * @sufile: inode of segment usage file
>> + * @buf: array of suinfo_update
>> + * @supsz: byte size of suinfo_update
>> + * @nsup: size of suinfo_update array
>> + *
>> + * Description: Takes an array of nilfs_suinfo_update structs and updates
>> + * segment usage accordingly. Only the fields indicated by the sup_flags
>> + * are updated.
>> + *
>> + * Return Value: On success, 0 is returned. On error, one of the
>> + * following negative error codes is returned.
>> + *
>> + * %-EIO - I/O error.
>> + *
>> + * %-ENOMEM - Insufficient amount of memory available.
>> + *
>> + * %-EINVAL - Invalid values in input (segment number, flags or nblocks)
>> + */
>> +ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
>> + unsigned supsz, size_t nsup)
>> +{
>> + struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
>> + struct buffer_head *header_bh, *bh;
>> + struct nilfs_suinfo_update *sup, *supend = buf + supsz * nsup;
>> + struct nilfs_segment_usage *su;
>> + void *kaddr;
>> + unsigned long blkoff, prev_blkoff;
>> + int ret = 0, ncleansegs, ndirtysegs, cleansi,
>> + cleansu, dirtysi, dirtysu;
>
> This indentation looks peculiar. Why not separate them into two or
> more lines? At least, ncleansegs and ndirtysegs differ from cleansi,
> cleansu, dirtysi, dirtysu and ret. ncleansegs and ndirtysegs sounds
> confusing since these local variables do not give a total number.
> ncleaned and ndirtied would be better.
>
> int cleansi, cleansu, dirtysi, dirtysu;
> int ncleaned, ndirtied;
> int ret = 0;
>
>> +
>> + if (unlikely(nsup == 0))
>> + return ret;
>> +
>> + for (sup = buf; sup < supend; sup = (void *)sup + supsz) {
>> + if (sup->sup_segnum >= nilfs->ns_nsegments
>> + || (sup->sup_flags &
>> + (~0UL << (NILFS_SUINFO_UPDATE_FLAGS + 1)))
>
> This looks confusing. It should be clarified as follows:
>
> enum {
> NILFS_SUINFO_UPDATE_LASTMOD,
> NILFS_SUINFO_UPDATE_NBLOCKS,
> NILFS_SUINFO_UPDATE_FLAGS,
> __NR_NILFS_SUINFO_FIELDS
> };
>
> if (sup->sup_segnum >= nilfs->ns_nsegments ||
> (sup->sup_flags & (~0UL << __NR_NILFS_SUINFO_FIELDS)) ||
>
>> + || (nilfs_suinfo_update_nblocks(sup) &&
>> + sup->sup_sui.sui_nblocks >
>> + nilfs->ns_blocks_per_segment)
>> + || (nilfs_suinfo_update_flags(sup) &&
>> + (sup->sup_sui.sui_flags &
>> + (~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
>
> Ditto. We need to add a definition to nilfs2_fs.h.
>
> enum {
> NILFS_SEGMENT_USAGE_ACTIVE,
> NILFS_SEGMENT_USAGE_DIRTY,
> NILFS_SEGMENT_USAGE_ERROR,
> __NR_NILFS_SEGMENT_USAGE_FLAGS
> };
>
> (nilfs_suinfo_update_flags(sup) &&
> (sup->sup_sui.sui_flags &
> (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
>
> By the way, this will dismiss the capability that userland cleaner
> programs uses the rest of su_flags for their own purpose such as GC
> optimization. I think this (rejecting or utilizing it) should be
> carefully determined.
>
> Any comments on this?
Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
As far as I can tell, it shouldn't affect the kernel side.
nilfs_segment_usage_set_clean() would still work and
nilfs_sufile_do_scrap() overwrites the whole su_flags as well.
>> + }
>> +
>> + down_write(&NILFS_MDT(sufile)->mi_sem);
>> +
>> + ret = nilfs_sufile_get_header_block(sufile, &header_bh);
>> + if (ret < 0)
>> + goto out_sem;
>> +
>> + sup = buf;
>> + blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
>> + ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
>> + if (ret < 0)
>> + goto out_header;
>> +
>> + for (;;) {
>> + kaddr = kmap_atomic(bh->b_page);
>> + su = nilfs_sufile_block_get_segment_usage(
>> + sufile, sup->sup_segnum, bh, kaddr);
>> +
>> + if (nilfs_suinfo_update_lastmod(sup))
>> + su->su_lastmod = cpu_to_le64(sup->sup_sui.sui_lastmod);
>> +
>> + if (nilfs_suinfo_update_nblocks(sup))
>> + su->su_nblocks = cpu_to_le32(sup->sup_sui.sui_nblocks);
>> +
>> + if (nilfs_suinfo_update_flags(sup)) {
>> + sup->sup_sui.sui_flags &=
>> + ~(1UL << NILFS_SEGMENT_USAGE_ACTIVE);
>
> Your would be better off adding a comment to explain what's this.
>
> /*
> * Active flag is a virtual flag projected by running
> * nilfs kernel code - drop it not to write it to
> * disk.
> */
>> +
>> + ncleansegs = 0;
>> + ndirtysegs = 0;
>> + cleansi = nilfs_suinfo_clean(&sup->sup_sui);
>> + cleansu = nilfs_segment_usage_clean(su);
>> + dirtysi = nilfs_suinfo_dirty(&sup->sup_sui);
>> + dirtysu = nilfs_segment_usage_dirty(su);
>> +
>> + if (cleansi && !cleansu)
>> + ++ncleansegs;
>> + else if (!cleansi && cleansu)
>> + --ncleansegs;
>> +
>> + if (dirtysi && !dirtysu)
>> + ++ndirtysegs;
>> + else if (!dirtysi && dirtysu)
>> + --ndirtysegs;
>> +
>> + su->su_flags = cpu_to_le32(sup->sup_sui.sui_flags);
>> +
>> + nilfs_sufile_mod_counter(header_bh, ncleansegs,
>> + ndirtysegs);
>
> Does it work for a negative value without cast of (u64) ?
> Please confirm that these counters are updated as you expected.
>
>> + NILFS_SUI(sufile)->ncleansegs += ncleansegs;
>
> Ditto.
I have tested it and it works. At least on my 64 bit architecture. It is
probably still a good idea to do an explicit cast.
How about I use s64 for ncleaned and ndirtied and move
nilfs_sufile_mod_counter outside the loop?
s64 ncleaned = 0, ndirtied = 0;
...
for (;;) {
...
}
mark_buffer_dirty(bh);
brelse(bh);
out_mark:
if (ncleaned || ndirtied) {
nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
(u64)ndirtied);
NILFS_SUI(sufile)->ncleansegs += ncleaned;
}
nilfs_mdt_mark_dirty(sufile);
out_header:
brelse(header_bh);
out_sem:
up_write(&NILFS_MDT(sufile)->mi_sem);
return ret;
Best regards,
Andreas Rohner
>
> Regards,
> Ryusuke Konishi
>
>> + }
>> +
>> + kunmap_atomic(kaddr);
>> +
>> + sup = (void *)sup + supsz;
>> + if (sup >= supend)
>> + break;
>> +
>> + prev_blkoff = blkoff;
>> + blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
>> + if (blkoff == prev_blkoff)
>> + continue;
>> +
>> + /* get different block */
>> + mark_buffer_dirty(bh);
>> + brelse(bh);
>> + ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
>> + if (unlikely(ret < 0))
>> + goto out_mark;
>> + }
>> + mark_buffer_dirty(bh);
>> + brelse(bh);
>> +
>> + out_mark:
>> + nilfs_mdt_mark_dirty(sufile);
>> + out_header:
>> + brelse(header_bh);
>> + out_sem:
>> + up_write(&NILFS_MDT(sufile)->mi_sem);
>> + return ret;
>> +}
>> +
>> +/**
>> * nilfs_sufile_read - read or get sufile inode
>> * @sb: super block instance
>> * @susize: size of a segment usage entry
>> diff --git a/fs/nilfs2/sufile.h b/fs/nilfs2/sufile.h
>> index e84bc5b..366003c 100644
>> --- a/fs/nilfs2/sufile.h
>> +++ b/fs/nilfs2/sufile.h
>> @@ -44,6 +44,7 @@ int nilfs_sufile_set_segment_usage(struct inode *sufile,
>> __u64 segnum,
>> int nilfs_sufile_get_stat(struct inode *, struct nilfs_sustat *);
>> 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,
>> --
>> 1.8.5.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
>> the body of a message to [email protected]
>> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
