[Ocfs2-devel] [PATCH 2/3] Ocfs2: Add a new code 'OCFS2_INFO_FREEINODE' for o2info ioctl.

[Mark Fasheh <mfasheh@suse.com>]

On Sun, Jan 30, 2011 at 02:26:00PM +0800, Tristan Ye wrote:
> The new code is dedicated to calculate free inodes number of all inode_allocs,
> then return the info to userpace in terms of an array.
> 
> Specially, flag 'OCFS2_INFO_FL_NON_COHERENT', manipulated by '--cluster-coherent'
> from userspace, is now going to be involved. setting the flag on means no cluster
> coherency considered, usually, userspace tools choose none-coherency strategy by
> default for the sake of performace.
> 
> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
> ---
>  fs/ocfs2/ioctl.c       |  122 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ocfs2/ocfs2_ioctl.h |   11 ++++
>  2 files changed, 133 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index 731cf46..18812be 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -23,6 +23,9 @@
>  #include "ioctl.h"
>  #include "resize.h"
>  #include "refcounttree.h"
> +#include "sysfile.h"
> +#include "dir.h"
> +#include "buffer_head_io.h"
>  
>  #include <linux/ext2_fs.h>
>  
> @@ -62,6 +65,13 @@ static inline void __o2info_clear_request_filled(struct ocfs2_info_request *req)
>  #define o2info_clear_request_filled(a) \
>  		__o2info_clear_request_filled((struct ocfs2_info_request *)&(a))
>  
> +static inline int __o2info_coherent(struct ocfs2_info_request *req)
> +{
> +	return (!(req->ir_flags & OCFS2_INFO_FL_NON_COHERENT));
> +}
> +
> +#define o2info_coherent(a) __o2info_coherent((struct ocfs2_info_request *)&(a))

Same comment about typechecking as before.


>  static int ocfs2_get_inode_attr(struct inode *inode, unsigned *flags)
>  {
>  	int status;
> @@ -321,6 +331,114 @@ bail:
>  	return status;
>  }
>  
> +int ocfs2_info_scan_inode_alloc(struct ocfs2_super *osb,
> +				struct inode *inode_alloc, u64 blkno,
> +				struct ocfs2_info_freeinode *fi, u32 slot)
> +{
> +	int status = 0, unlock = 0;
> +
> +	struct buffer_head *bh = NULL;
> +	struct ocfs2_dinode *dinode_alloc = NULL;
> +
> +	if (inode_alloc)
> +		mutex_lock(&inode_alloc->i_mutex);
> +
> +	if (o2info_coherent(*fi)) {
> +		status = ocfs2_inode_lock(inode_alloc, &bh, 0);
> +		if (status < 0) {
> +			mlog_errno(status);
> +			goto bail;
> +		}
> +		unlock = 1;
> +	} else {
> +		status = ocfs2_read_blocks_sync(osb, blkno, 1, &bh);
> +		if (status < 0) {
> +			mlog_errno(status);
> +			goto bail;
> +		}
> +	}
> +
> +	dinode_alloc = (struct ocfs2_dinode *)bh->b_data;
> +
> +	fi->ifi_stat[slot].lfi_total =
> +		le32_to_cpu(dinode_alloc->id1.bitmap1.i_total);
> +	fi->ifi_stat[slot].lfi_free =
> +		le32_to_cpu(dinode_alloc->id1.bitmap1.i_total) -
> +		le32_to_cpu(dinode_alloc->id1.bitmap1.i_used);
> +
> +bail:
> +	if (unlock)
> +		ocfs2_inode_unlock(inode_alloc, 0);
> +
> +	if (inode_alloc)
> +		mutex_unlock(&inode_alloc->i_mutex);
> +
> +	if (inode_alloc)
> +		iput(inode_alloc);
    		^^^^^^^^^^^^^^^^^^

Is this iput necessary? I don't see a balancing ref in the function.

> +
> +	brelse(bh);
> +
> +	mlog_exit(status);
> +	return status;
> +}
> +
> +int ocfs2_info_handle_freeinode(struct inode *inode,
> +				struct ocfs2_info_request __user *req)
> +{
> +	u32 i;
> +	u64 blkno = -1;
> +	char namebuf[40];
> +	int status = -EFAULT, type = INODE_ALLOC_SYSTEM_INODE;
> +	struct ocfs2_info_freeinode oifi;
> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> +	struct inode *inode_alloc = NULL;
> +
> +	if (o2info_from_user(oifi, req))
> +		goto bail;
> +
> +	oifi.ifi_slotnum = osb->max_slots;
> +
> +	for (i = 0; i < oifi.ifi_slotnum; i++) {
> +		if (o2info_coherent(oifi)) {
> +			inode_alloc = ocfs2_get_system_file_inode(osb, type, i);
> +			if (!inode_alloc) {
> +				mlog(ML_ERROR, "unable to get alloc inode in "
> +				    "slot %u\n", i);
> +				status = -EIO;
> +				goto bail;
> +			}
> +		} else {
> +			ocfs2_sprintf_system_inode_name(namebuf,
> +							sizeof(namebuf),
> +							type, i);
> +			status = ocfs2_lookup_ino_from_name(osb->sys_root_inode,
> +							    namebuf,
> +							    strlen(namebuf),
> +							    &blkno);
> +			if (status < 0) {
> +				status = -ENOENT;
> +				goto bail;
> +			}
> +		}
> +
> +		status = ocfs2_info_scan_inode_alloc(osb, inode_alloc, blkno, &oifi, i);

You need the following here:

		iput(inode_alloc);
		inode_alloc = NULL;

> +		if (status < 0)
> +			goto bail;
> +	}
> +
> +	o2info_set_request_filled(oifi);
> +
> +	if (o2info_to_user(oifi, req))
> +		goto bail;
> +
> +	status = 0;
> +bail:
> +	if (status)
> +		o2info_set_request_error(oifi, req);
> +
> +	return status;
> +}
> +
>  int ocfs2_info_handle_unknown(struct inode *inode,
>  			      struct ocfs2_info_request __user *req)
>  {
> @@ -392,6 +510,10 @@ int ocfs2_info_handle_request(struct inode *inode,
>  		if (oir.ir_size == sizeof(struct ocfs2_info_journal_size))
>  			status = ocfs2_info_handle_journal_size(inode, req);
>  		break;
> +	case OCFS2_INFO_FREEINODE:
> +		if (oir.ir_size == sizeof(struct ocfs2_info_freeinode))
> +			status = ocfs2_info_handle_freeinode(inode, req);
> +		break;
>  	default:
>  		status = ocfs2_info_handle_unknown(inode, req);
>  		break;
> diff --git a/fs/ocfs2/ocfs2_ioctl.h b/fs/ocfs2/ocfs2_ioctl.h
> index b46f39b..6b4b39a 100644
> --- a/fs/ocfs2/ocfs2_ioctl.h
> +++ b/fs/ocfs2/ocfs2_ioctl.h
> @@ -142,6 +142,16 @@ struct ocfs2_info_journal_size {
>  	__u64 ij_journal_size;
>  };
>  
> +struct ocfs2_info_freeinode {
> +	struct ocfs2_info_request ifi_req;
> +	struct ocfs2_info_local_freeinode {
> +		__u64 lfi_total;
> +		__u64 lfi_free;
> +	} ifi_stat[OCFS2_MAX_SLOTS];
> +	__u32 ifi_slotnum; /* out */
> +	__u32 ifi_pad;
> +};

Any reason why we don't keep the array (ocfs2_info_local_freeinode) at the
end of this structure like this:

struct ocfs2_info_freeinode {
	struct ocfs2_info_request ifi_req;
	__u32 ifi_slotnum; /* out */
	__u32 ifi_pad;
	struct ocfs2_info_local_freeinode {
		__u64 lfi_total;
		__u64 lfi_free;
	} ifi_stat[OCFS2_MAX_SLOTS];
};
	--Mark

--
Mark Fasheh