| 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
| ||
|
Message-ID: <49D1DB04.3040505@panasas.com>
Date: Tue, 31 Mar 2009 11:57:40 +0300
From: Boaz Harrosh <bharrosh@...asas.com>
To: Andrew Morton <akpm@...ux-foundation.org>
CC: Avishay Traeger <avishay@...il.com>, Jeff Garzik <jeff@...zik.org>,
Evgeniy Polyakov <zbr@...emap.net>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
open-osd <osd-dev@...n-osd.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
James Bottomley <James.Bottomley@...senPartnership.com>,
FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
Subject: Re: [PATCH 1/8] exofs: Kbuild, Headers and osd utils
On 03/31/2009 11:04 AM, Andrew Morton wrote:
> On Wed, 18 Mar 2009 19:57:36 +0200 Boaz Harrosh <bharrosh@...asas.com> wrote:
>
>> This patch includes osd infrastructure that will be used later by
>> the file system.
>>
>> Also the declarations of constants, on disk structures,
>> and prototypes.
>>
>> And the Kbuild+Kconfig files needed to build the exofs module.
>>
>> ...
>>
>> --- /dev/null
>> +++ b/fs/exofs/Kbuild
>> @@ -0,0 +1,30 @@
>> +#
>> +# Kbuild for the EXOFS module
>> +#
>> +# Copyright (C) 2008 Panasas Inc. All rights reserved.
>> +#
>> +# Authors:
>> +# Boaz Harrosh <bharrosh@...asas.com>
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License version 2
>> +#
>> +# Kbuild - Gets included from the Kernels Makefile and build system
>> +#
>> +
>> +ifneq ($(OSD_INC),)
>> +# we are built out-of-tree Kconfigure everything as on
>> +
>> +CONFIG_EXOFS_FS=m
>> +ccflags-y += -DCONFIG_EXOFS_FS -DCONFIG_EXOFS_FS_MODULE
>> +# ccflags-y += -DCONFIG_EXOFS_DEBUG
>> +
>> +# if we are built out-of-tree and the hosting kernel has OSD headers
>> +# then "ccflags-y +=" will not pick the out-off-tree headers. Only by doing
>> +# this it will work. This might break in future kernels
>> +KBUILD_CPPFLAGS := -I$(OSD_INC) $(KBUILD_CPPFLAGS)
>> +
>> +endif
>
> But this patch is putting the fs into the tree, so all the above is unneeded.
>
>> ...
>>
>> + * Object IDs 0, 1, and 2 are always in use (see above defines).
>> + */
>> +enum {
>> + EXOFS_UINT64_MAX = (~0LL),
>
> Use ULLONG_MAX?
>
> ~0ULL would be more consistent.
>
>> + EXOFS_MAX_INO_ID = (sizeof(ino_t) * 8 == 64) ? EXOFS_UINT64_MAX :
>> + (1LL << (sizeof(ino_t) * 8 - 1)),
>
> Tricky, needs a comment.
>
> Would be clearer to use 1ULL.
>
>> + EXOFS_MAX_ID = (EXOFS_MAX_INO_ID - 1 - EXOFS_OBJ_OFF),
>> +};
>> +
OK, OK, OK
>> +/****************************************************************************
>> + * Misc.
>> + ****************************************************************************/
>> +#define EXOFS_BLKSHIFT 12
>> +#define EXOFS_BLKSIZE (1UL << EXOFS_BLKSHIFT)
>> +
>> +/****************************************************************************
>> + * superblock-related things
>> + ****************************************************************************/
>> +#define EXOFS_SUPER_MAGIC 0x5DF5
>
> Should be in include/linux/magic.h
>
Is this relevant for OSD, I guess if there are going to
be more OSD filesystems then yes.
I will do it, thanks.
>> ...
>>
>> +/*
>> + * The file control block - stored in an object's attributes. This is where
>> + * the in-memory inode is stored on disk.
>> + */
>> +struct exofs_fcb {
>> + __le64 i_size; /* Size of the file */
>> + __le16 i_mode; /* File mode */
>> + __le16 i_links_count; /* Links count */
>> + __le32 i_uid; /* Owner Uid */
>> + __le32 i_gid; /* Group Id */
>> + __le32 i_atime; /* Access time */
>> + __le32 i_ctime; /* Creation time */
>> + __le32 i_mtime; /* Modification time */
>> + __le32 i_flags; /* File flags (unused for now)*/
>> + __le32 i_generation; /* File version (for NFS) */
>> + __le32 i_data[EXOFS_IDATA]; /* Short symlink names and device #s */
>> +};
>
> There is no room for future expansion. Would that be appropriate/wise?
> I guess it would need versioning information somewhere too.
>
In osd we have the size-of-the-attribute it sits in. So if in future we
add members we can switch according to size, also we can just stick it in
a different attribute number, so like EXOFS_ATTR_INODE_DATA_VER1
EXOFS_ATTR_INODE_DATA_VER2 attribute. Presence of, means support. Hell we can
even be backward compatible with having 2 or three versions at once.
>> ...
>>
>> +/* u64 has problems with printk this will cast it to unsigned long long */
>> +#define _LLU(x) (unsigned long long)(x)
>
> ug.
>
> Normally the response is "please open-code this". But given that one
> day real soon this printk(u64) problem will be fixed, I guess the use
> of _LLU will make it easy to find and delete all the now-unneeded
> casts.
>
Exactly my thoughts
>> ...
>>
>> +/*
>> + * our inode flags
>> + */
>> +#define OBJ_2BCREATED 0 /* object will be created soon*/
>> +#define OBJ_CREATED 1 /* object has been created on the osd*/
>> +
>> +static inline int obj_2bcreated(struct exofs_i_info *oi)
>> +{
>> + return test_bit(OBJ_2BCREATED, &(oi->i_flags));
>> +}
>
> unneeded parentheses around oi->i_flags.
>
>> +static inline void set_obj_2bcreated(struct exofs_i_info *oi)
>> +{
>> + set_bit(OBJ_2BCREATED, &(oi->i_flags));
>> +}
>> +
>> +static inline int obj_created(struct exofs_i_info *oi)
>> +{
>> + return test_bit(OBJ_CREATED, &(oi->i_flags));
>> +}
>> +
>> +static inline void set_obj_created(struct exofs_i_info *oi)
>> +{
>> + set_bit(OBJ_CREATED, &(oi->i_flags));
>> +}
>
> dittoes.
>
>> ...
>>
>
Thanks
will fix
Boaz
--
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/
Powered by blists - more mailing lists