On Thu, Jan 10, 2013 at 8:48 AM, David Sterba <dsterba@xxxxxxx> wrote:
> On Thu, Dec 20, 2012 at 10:43:20PM +0800, zwu.kernel@xxxxxxxxx wrote:
>> --- /dev/null
>> +++ b/fs/hot_tracking.c
>> @@ -0,0 +1,109 @@
>> +/*
>> + * fs/hot_tracking.c
>
> From what I've undrestood the file name written here is not wanted, so
> please drop it (and from .h too)
Done.
>
>> + *
>> + * Copyright (C) 2012 IBM Corp. All rights reserved.
>> + * Written by Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public
>> + * License v2 as published by the Free Software Foundation.
>
> A short description of the hot tracking feature or pointer to the
> Documentation/ file would be nice here.
ok, Done
>
>> + */
>> +
>> +#include <linux/list.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/hardirq.h>
>> +#include <linux/fs.h>
>> +#include <linux/blkdev.h>
>> +#include <linux/types.h>
>> +#include <linux/limits.h>
>> +#include "hot_tracking.h"
>> +
>> +/* kmem_cache pointers for slab caches */
>
> This comment seems useless to me, I does not help understanding the code, just
> says the same what reads in C. There are more such redundant comments in the
> series, but I'm not going point to all of them right now.
Removed.
>
>> +static struct kmem_cache *hot_inode_item_cachep __read_mostly;
>> +static struct kmem_cache *hot_range_item_cachep __read_mostly;
>> +
>
>> --- /dev/null
>> +++ b/include/linux/hot_tracking.h
>> +/* The common info for both following structures */
>> +struct hot_comm_item {
>> + struct rb_node rb_node; /* rbtree index */
>> + struct hot_freq_data hot_freq_data; /* frequency data */
>> + spinlock_t lock; /* protects object data */
>> + struct kref refs; /* prevents kfree */
>> +};
>> +
>> +/* An item representing an inode and its access frequency */
>> +struct hot_inode_item {
>> + struct hot_comm_item hot_inode; /* node in hot_inode_tree */
>> + struct hot_rb_tree hot_range_tree; /* tree of ranges */
>> + spinlock_t lock; /* protect range tree */
>> + struct hot_rb_tree *hot_inode_tree;
>> + u64 i_ino; /* inode number from inode */
>> +};
>
> Please align the comments to something like this (or drop them if they seem
> redundant):
Done
>
> /* The common info for both following structures */
> struct hot_comm_item {
> struct rb_node rb_node; /* rbtree index */
> struct hot_freq_data hot_freq_data; /* frequency data */
> spinlock_t lock; /* protects object data */
> struct kref refs; /* prevents kfree */
> struct list_head n_list; /* list node index */
> };
>
> /* An item representing an inode and its access frequency */
> struct hot_inode_item {
> struct hot_comm_item hot_inode; /* node in hot_inode_tree */
> struct hot_rb_tree hot_range_tree; /* tree of ranges */
> spinlock_t lock; /* protect range tree */
> struct hot_rb_tree *hot_inode_tree;
> u64 i_ino; /* inode number from inode */
> };
>
>> +extern void __init hot_cache_init(void);
>
> this belongs to the private include fs/hot_tracking.h (because this is called
> only once by vfs init and not by filesystems), there's
> hot_track_init(superblock) for that purpose introduced later.
Done, Move it to fs/hot_tracking.h
>
>
> david
--
Regards,
Zhi Yong Wu
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
