Re: [PATCH 1/9] exofs: osd Swiss army knife

Andrew Morton
2008-12-29T20:30:30Z

On Tue, 16 Dec 2008 16:52:54 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> In this patch are all the 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.
>
>
> ...
>
> +struct exofs_sb_info {
> + struct osd_dev *s_dev; /* returned by get_osd_dev */
> + uint64_t s_pid; /* partition ID of file system*/
> + int s_timeout; /* timeout for OSD operations */
> + uint32_t s_nextid; /* highest object ID used */
> + uint32_t s_numfiles; /* number of files on fs */
> + spinlock_t s_next_gen_lock; /* spinlock for gen # update */
> + u32 s_next_generation; /* next gen # to use */
> + atomic_t s_curr_pending; /* number of pending commands */
> + uint8_t s_cred[OSD_CAP_LEN]; /* all-powerful credential */
> +};
> +
> +/*
> + * our inode flags
> + */
> +#ifdef ARCH_HAS_ATOMIC_UNSIGNED

This doesn't exist, and it would be fairly bad to introduce it. Please
kill the ifdefs.

> +typedef unsigned exofs_iflags_t;
> +#else
> +typedef unsigned long exofs_iflags_t;
> +#endif

Then please kill the typedef altogether and replace it with `unsigned
long' everywhere.

> +#define OBJ_2BCREATED 0 /* object will be created soon*/
> +#define OBJ_CREATED 1 /* object has been created on the osd*/
> +
> +#define Obj2BCreated(oi) \
> + test_bit(OBJ_2BCREATED, &(oi->i_flags))
> +#define SetObj2BCreated(oi) \
> + set_bit(OBJ_2BCREATED, &(oi->i_flags))
> +
> +#define ObjCreated(oi) \
> + test_bit(OBJ_CREATED, &(oi->i_flags))
> +#define SetObjCreated(oi) \
> + set_bit(OBJ_CREATED, &(oi->i_flags))

- please only implement code in macros when it CANNOT be implemented
in C. There are numerous reasons. One of which is that the above
macros will happily compile when passed a pointer to ANY truct whcih
has an i_flags field. If it were a properly typechecked C function,
that can't happen.

- These "functions" have odd names. This:

static inline void obj_created(struct exofs_i_info *ei)

would be more Linux-like.

> +/*
> + * our extension to the in-memory inode
> + */
> +struct exofs_i_info {
> + exofs_iflags_t i_flags; /* various atomic flags */
> + __le32 i_data[EXOFS_IDATA];/*short symlink names and device #s*/
> + uint32_t i_dir_start_lookup; /* which page to start lookup */
> + wait_queue_head_t i_wq; /* wait queue for inode */
> + uint64_t i_commit_size; /* the object's written length */
> + uint8_t i_cred[OSD_CAP_LEN];/* all-powerful credential */
> + struct inode vfs_inode; /* normal in-memory inode */
> +};
> +
> +/*
> + * get to our inode from the vfs inode
> + */
> +static inline struct exofs_i_info *EXOFS_I(struct inode *inode)
> +{
> + return container_of(inode, struct exofs_i_info, vfs_inode);
> +}

yeah, well. We got lazy when, we converted EXT2_I from a macro to a C
function. That doesn't mean that the mistake should have been copied :)
exofs_i() would be a more suitable name.

> +/*************************
> + * function declarations *
> + *************************/
>
> ...
>
> +#include <scsi/scsi_device.h>
> +#include <scsi/osd_sense.h>
> +
> +#include "exofs.h"
> +
> +int check_ok(struct osd_request *req)

eek. This is a kernel-wide symbol. The choice of identifier is bad.

> +{
> + struct osd_sense_info osi;
> + int ret = osd_req_decode_sense(req, &osi);
> +
> + if (ret) { /* translate to Linux codes */
> + if (osi.additional_code == scsi_invalid_field_in_cdb) {
> + if (osi.cdb_field_offset == OSD_CFO_STARTING_BYTE)
> + ret = -EFAULT;
> + if (osi.cdb_field_offset == OSD_CFO_OBJECT_ID)
> + ret = -ENOENT;
> + else
> + ret = -EINVAL;
> + } else if (osi.additional_code == osd_quota_error)
> + ret = -ENOSPC;
> + else
> + ret = -EIO;
> + }
> +
> + return ret;
> +}
> +
> +void make_credential(uint8_t cred_a[OSD_CAP_LEN], uint64_t pid, uint64_t oid)

Ditto. I suspect I'm going to see a lot of this. Please review the
entire fs for its namespace niceness

> +{
> + struct osd_obj_id obj = {
> + .partition = pid,
> + .id = oid
> + };
> +
> + osd_sec_init_nosec_doall_caps(cred_a, &obj, false, true);
> +}
> +
>
> ...
>
> +int prepare_get_attr_list_add_entry(struct osd_request *req,
> + uint32_t page_num,
> + uint32_t attr_num,
> + uint32_t attr_len)
> +{
> + struct osd_attr attr = {
> + .page = page_num,

Kernel developers expect a field called "page" to have type `struct
page *'. osd_attr.page is thus designed to confuse.

>
> ...
>