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 <andreas.rohner@xxxxxxx>
> ---
> 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 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
