On Mar 16, 2014, at 1:47 PM, Andreas Rohner wrote: > This patch adds an additional timestamp to the segment usage > information that indicates the last time the usage information was > changed. So su_lastmod indicates the last time the segment itself was > modified and su_lastdec indicates the last time the usage information > itself was changed. >
What will we have if user changes time? What sequence will we have after such "malicious" action? Did you test such situation? > This is important information for the GC, because it needs to avoid > selecting segments for cleaning that are created (su_lastmod) outside of > the protection period, but the blocks got reclaimable (su_nblocks is > decremented) within the protection period. Without that information the > GC policy has to assume, that there are reclaimble blocks, only to find > out, that they are protected by the protection period. > > This patch also introduces nilfs_sufile_add_segment_usage(), which can > be used to increment or decrement the value of su_nblocks of a specific > segment. > > Signed-off-by: Andreas Rohner <[email protected]> > --- > fs/nilfs2/sufile.c | 86 +++++++++++++++++++++++++++++++++++++++++++++-- > fs/nilfs2/sufile.h | 18 ++++++++++ > include/linux/nilfs2_fs.h | 7 ++++ > 3 files changed, 109 insertions(+), 2 deletions(-) > > diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c > index 2a869c3..0886938 100644 > --- a/fs/nilfs2/sufile.c > +++ b/fs/nilfs2/sufile.c > @@ -453,6 +453,8 @@ void nilfs_sufile_do_scrap(struct inode *sufile, __u64 > segnum, > su->su_lastmod = cpu_to_le64(0); > su->su_nblocks = cpu_to_le32(0); > su->su_flags = cpu_to_le32(1UL << NILFS_SEGMENT_USAGE_DIRTY); > + if (nilfs_sufile_lastdec_supported(sufile)) > + su->su_lastdec = cpu_to_le64(0); > kunmap_atomic(kaddr); > > nilfs_sufile_mod_counter(header_bh, clean ? (u64)-1 : 0, dirty ? 0 : 1); > @@ -482,7 +484,7 @@ void nilfs_sufile_do_free(struct inode *sufile, __u64 > segnum, > WARN_ON(!nilfs_segment_usage_dirty(su)); > > sudirty = nilfs_segment_usage_dirty(su); > - nilfs_segment_usage_set_clean(su); > + nilfs_sufile_segment_usage_set_clean(sufile, su); > kunmap_atomic(kaddr); > mark_buffer_dirty(su_bh); > > @@ -549,6 +551,75 @@ int nilfs_sufile_set_segment_usage(struct inode *sufile, > __u64 segnum, > } > > /** > + * nilfs_sufile_add_segment_usage - decrement usage of a segment I feel cultural dissonance about this name. Add or decrement? :) Decrement and add are different operations for me. > + * @sufile: inode of segment usage file > + * @segnum: segment number > + * @value: value to add to su_nblocks > + * @dectime: current time > + * > + * Description: nilfs_sufile_add_segment_usage() adds a signed value to the > + * su_nblocks field of the segment usage information of @segnum. It ensures > + * that the result is bigger than 0 and smaller or equal to the maximum > number > + * of blocks per segment > + * > + * Return Value: On success, 0 is returned. On error, one of the following > + * negative error codes is returned. > + * > + * %-ENOMEM - Insufficient memory available. > + * > + * %-EIO - I/O error > + * > + * %-ENOENT - the specified block does not exist (hole block) > + */ > +int nilfs_sufile_add_segment_usage(struct inode *sufile, __u64 segnum, > + __s64 value, time_t dectime) > +{ > + struct the_nilfs *nilfs = sufile->i_sb->s_fs_info; > + struct buffer_head *bh; > + struct nilfs_segment_usage *su; > + void *kaddr; > + int ret; > + > + if (value == 0) > + return 0; > + > + down_write(&NILFS_MDT(sufile)->mi_sem); > + > + ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0, &bh); > + if (ret < 0) Maybe it needs to use unlikely() here. > + goto out_sem; > + > + kaddr = kmap_atomic(bh->b_page); > + su = nilfs_sufile_block_get_segment_usage(sufile, segnum, bh, kaddr); > + WARN_ON(nilfs_segment_usage_error(su)); > + > + value += le32_to_cpu(su->su_nblocks); Decrement. Really? :) > + if (value < 0) > + value = 0; > + if (value > nilfs->ns_blocks_per_segment) maybe "else if" here? > + value = nilfs->ns_blocks_per_segment; > + > + if (value == le32_to_cpu(su->su_nblocks)) { > + kunmap_atomic(kaddr); > + goto out_brelse; > + } > + > + su->su_nblocks = cpu_to_le32(value); > + if (dectime && nilfs_sufile_lastdec_supported(sufile)) > + su->su_lastdec = cpu_to_le64(dectime); > + kunmap_atomic(kaddr); > + > + mark_buffer_dirty(bh); > + nilfs_mdt_mark_dirty(sufile); > + > +out_brelse: > + brelse(bh); > +out_sem: > + up_write(&NILFS_MDT(sufile)->mi_sem); > + return ret; > +} > + > +/** > * nilfs_sufile_get_stat - get segment usage statistics > * @sufile: inode of segment usage file > * @stat: pointer to a structure of segment usage statistics > @@ -698,7 +769,8 @@ static int nilfs_sufile_truncate_range(struct inode > *sufile, > nc = 0; > for (su = su2, j = 0; j < n; j++, su = (void *)su + susz) { > if (nilfs_segment_usage_error(su)) { > - nilfs_segment_usage_set_clean(su); > + nilfs_sufile_segment_usage_set_clean(sufile, > + su); > nc++; > } > } > @@ -858,6 +930,13 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, > __u64 segnum, void *buf, > if (nilfs_segment_is_active(nilfs, segnum + j)) > si->sui_flags |= > (1UL << NILFS_SEGMENT_USAGE_ACTIVE); > + if (sisz >= sizeof(struct nilfs_suinfo)) { > + if (susz >= sizeof(struct nilfs_segment_usage)) > + si->sui_lastdec = > + le64_to_cpu(su->su_lastdec); Is it really impossible to place assignment on one line? > + else > + si->sui_lastdec = 0; > + } > } > kunmap_atomic(kaddr); > brelse(su_bh); > @@ -935,6 +1014,9 @@ ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, > void *buf, > if (nilfs_suinfo_update_lastmod(sup)) > su->su_lastmod = cpu_to_le64(sup->sup_sui.sui_lastmod); > > + if (nilfs_suinfo_update_lastdec(sup)) > + su->su_lastdec = cpu_to_le64(sup->sup_sui.sui_lastdec); > + > if (nilfs_suinfo_update_nblocks(sup)) > su->su_nblocks = cpu_to_le32(sup->sup_sui.sui_nblocks); > > diff --git a/fs/nilfs2/sufile.h b/fs/nilfs2/sufile.h > index b8afd72..e5455d2 100644 > --- a/fs/nilfs2/sufile.h > +++ b/fs/nilfs2/sufile.h > @@ -28,6 +28,23 @@ > #include <linux/nilfs2_fs.h> > #include "mdt.h" > > +static inline int > +nilfs_sufile_lastdec_supported(const struct inode *sufile) > +{ > + return NILFS_MDT(sufile)->mi_entry_size == > + sizeof(struct nilfs_segment_usage); > +} > + > +static inline void > +nilfs_sufile_segment_usage_set_clean(const struct inode *sufile, > + struct nilfs_segment_usage *su) > +{ > + su->su_lastmod = cpu_to_le64(0); > + su->su_nblocks = cpu_to_le32(0); > + su->su_flags = cpu_to_le32(0); > + if (nilfs_sufile_lastdec_supported(sufile)) > + su->su_lastdec = cpu_to_le64(0); > +} > > static inline unsigned long nilfs_sufile_get_nsegments(struct inode *sufile) > { > @@ -41,6 +58,7 @@ int nilfs_sufile_alloc(struct inode *, __u64 *); > int nilfs_sufile_mark_dirty(struct inode *sufile, __u64 segnum); > int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum, > unsigned long nblocks, time_t modtime); > +int nilfs_sufile_add_segment_usage(struct inode *, __u64, __s64, time_t); > int nilfs_sufile_get_stat(struct inode *, struct nilfs_sustat *); > ssize_t nilfs_sufile_get_suinfo(struct inode *, __u64, void *, unsigned, > size_t); > diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h > index ff3fea3..ca269ad 100644 > --- a/include/linux/nilfs2_fs.h > +++ b/include/linux/nilfs2_fs.h > @@ -614,11 +614,13 @@ struct nilfs_cpfile_header { > * @su_lastmod: last modified timestamp > * @su_nblocks: number of blocks in segment > * @su_flags: flags > + * @su_lastdec: last decrement of su_nblocks timestamp > */ > struct nilfs_segment_usage { > __le64 su_lastmod; > __le32 su_nblocks; > __le32 su_flags; > + __le64 su_lastdec; So, this change makes on-disk layout incompatible with previous one. Am I correct? At first it needs to be fully confident that we really need in changing in this place. Secondly, it needs to add incompatible flag for s_feature_incompat field of superblock and maybe mount option. The su_lastdec sounds not very good for my taste. Thanks, Vyacheslav Dubeyko. > }; > > #define NILFS_MIN_SEGMENT_USAGE_SIZE 16 > @@ -663,6 +665,7 @@ nilfs_segment_usage_set_clean(struct nilfs_segment_usage > *su) > su->su_lastmod = cpu_to_le64(0); > su->su_nblocks = cpu_to_le32(0); > su->su_flags = cpu_to_le32(0); > + su->su_lastdec = cpu_to_le64(0); > } > > static inline int > @@ -694,11 +697,13 @@ struct nilfs_sufile_header { > * @sui_lastmod: timestamp of last modification > * @sui_nblocks: number of written blocks in segment > * @sui_flags: segment usage flags > + * @sui_lastdec: last decrement of sui_nblocks timestamp > */ > struct nilfs_suinfo { > __u64 sui_lastmod; > __u32 sui_nblocks; > __u32 sui_flags; > + __u64 sui_lastdec; > }; > > #define NILFS_SUINFO_FNS(flag, name) \ > @@ -736,6 +741,7 @@ enum { > NILFS_SUINFO_UPDATE_LASTMOD, > NILFS_SUINFO_UPDATE_NBLOCKS, > NILFS_SUINFO_UPDATE_FLAGS, > + NILFS_SUINFO_UPDATE_LASTDEC, > __NR_NILFS_SUINFO_UPDATE_FIELDS, > }; > > @@ -759,6 +765,7 @@ nilfs_suinfo_update_##name(const struct > nilfs_suinfo_update *sup) \ > NILFS_SUINFO_UPDATE_FNS(LASTMOD, lastmod) > NILFS_SUINFO_UPDATE_FNS(NBLOCKS, nblocks) > NILFS_SUINFO_UPDATE_FNS(FLAGS, flags) > +NILFS_SUINFO_UPDATE_FNS(LASTDEC, lastdec) > > enum { > NILFS_CHECKPOINT, > -- > 1.9.0 > > -- > 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
