linux-kernel - Re: [PATCH RESEND v1 01/16] vfs: introduce some data structures

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 10 Jan 2013 14:24:50 +0800
From:	Zhi Yong Wu <zwu.kernel@...il.com>
To:	dsterba@...e.cz
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	viro@...iv.linux.org.uk, david@...morbit.com,
	darrick.wong@...cle.com, andi@...stfloor.org, hch@...radead.org,
	linuxram@...ux.vnet.ibm.com, wuzhy@...ux.vnet.ibm.com
Subject: Re: [PATCH RESEND v1 01/16] vfs: introduce some data structures

On Thu, Jan 10, 2013 at 8:48 AM, David Sterba <dsterba@...e.cz> wrote:
> On Thu, Dec 20, 2012 at 10:43:20PM +0800, zwu.kernel@...il.com 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@...ux.vnet.ibm.com>
>> + *
>> + * 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/