[Ocfs2-devel] [PATCH 0/3] Ocfs2: Adding new codes 'OCFS2_INFO_FREEINODE' and 'OCFS2_INFO_FREEFRAG' for o2info ioctl V2.

[Ocfs2-devel] [PATCH 0/3] Ocfs2: Adding new codes 'OCFS2_INFO_FREEINODE' and 'OCFS2_INFO_FREEFRAG' for o2info ioctl V2.

[Tristan Ye]

This patch set is an attempt to add the last two codes for OCFS2_IOC_INFO, it
hopes with luck, to be committed upstream in next merge-window.

It has been in following remote branch:

git://oss.oracle.com/git/tye/linux-2.6.git o2info

Tristan

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Tristan Ye]

It's a best-effort attempt to simplize duplicated codes here.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/ioctl.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index 7a48681..731cf46 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -46,6 +46,22 @@ static inline void __o2info_set_request_error(struct ocfs2_info_request *kreq,
 #define o2info_set_request_error(a, b) \
 		__o2info_set_request_error((struct ocfs2_info_request *)&(a), b)
 
+static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
+{
+	req->ir_flags |= OCFS2_INFO_FL_FILLED;
+}
+
+#define o2info_set_request_filled(a) \
+		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
+
+static inline void __o2info_clear_request_filled(struct ocfs2_info_request *req)
+{
+	req->ir_flags &= ~OCFS2_INFO_FL_FILLED;
+}
+
+#define o2info_clear_request_filled(a) \
+		__o2info_clear_request_filled((struct ocfs2_info_request *)&(a))
+
 static int ocfs2_get_inode_attr(struct inode *inode, unsigned *flags)
 {
 	int status;
@@ -139,7 +155,8 @@ int ocfs2_info_handle_blocksize(struct inode *inode,
 		goto bail;
 
 	oib.ib_blocksize = inode->i_sb->s_blocksize;
-	oib.ib_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+
+	o2info_set_request_filled(oib);
 
 	if (o2info_to_user(oib, req))
 		goto bail;
@@ -163,7 +180,8 @@ int ocfs2_info_handle_clustersize(struct inode *inode,
 		goto bail;
 
 	oic.ic_clustersize = osb->s_clustersize;
-	oic.ic_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+
+	o2info_set_request_filled(oic);
 
 	if (o2info_to_user(oic, req))
 		goto bail;
@@ -187,7 +205,8 @@ int ocfs2_info_handle_maxslots(struct inode *inode,
 		goto bail;
 
 	oim.im_max_slots = osb->max_slots;
-	oim.im_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+
+	o2info_set_request_filled(oim);
 
 	if (o2info_to_user(oim, req))
 		goto bail;
@@ -211,7 +230,8 @@ int ocfs2_info_handle_label(struct inode *inode,
 		goto bail;
 
 	memcpy(oil.il_label, osb->vol_label, OCFS2_MAX_VOL_LABEL_LEN);
-	oil.il_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+
+	o2info_set_request_filled(oil);
 
 	if (o2info_to_user(oil, req))
 		goto bail;
@@ -235,7 +255,8 @@ int ocfs2_info_handle_uuid(struct inode *inode,
 		goto bail;
 
 	memcpy(oiu.iu_uuid_str, osb->uuid_str, OCFS2_TEXT_UUID_LEN + 1);
-	oiu.iu_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+
+	o2info_set_request_filled(oiu);
 
 	if (o2info_to_user(oiu, req))
 		goto bail;
@@ -261,7 +282,8 @@ int ocfs2_info_handle_fs_features(struct inode *inode,
 	oif.if_compat_features = osb->s_feature_compat;
 	oif.if_incompat_features = osb->s_feature_incompat;
 	oif.if_ro_compat_features = osb->s_feature_ro_compat;
-	oif.if_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+
+	o2info_set_request_filled(oif);
 
 	if (o2info_to_user(oif, req))
 		goto bail;
@@ -286,7 +308,7 @@ int ocfs2_info_handle_journal_size(struct inode *inode,
 
 	oij.ij_journal_size = osb->journal->j_inode->i_size;
 
-	oij.ij_req.ir_flags |= OCFS2_INFO_FL_FILLED;
+	o2info_set_request_filled(oij);
 
 	if (o2info_to_user(oij, req))
 		goto bail;
@@ -308,7 +330,7 @@ int ocfs2_info_handle_unknown(struct inode *inode,
 	if (o2info_from_user(oir, req))
 		goto bail;
 
-	oir.ir_flags &= ~OCFS2_INFO_FL_FILLED;
+	o2info_clear_request_filled(oir);
 
 	if (o2info_to_user(oir, req))
 		goto bail;
-- 
1.5.5

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

[Tristan Ye]

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))
+
 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);
+
+	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);
+		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;
+};
+
 /* Codes for ocfs2_info_request */
 enum ocfs2_info_type {
 	OCFS2_INFO_CLUSTERSIZE = 1,
@@ -151,6 +161,7 @@ enum ocfs2_info_type {
 	OCFS2_INFO_UUID,
 	OCFS2_INFO_FS_FEATURES,
 	OCFS2_INFO_JOURNAL_SIZE,
+	OCFS2_INFO_FREEINODE,
 	OCFS2_INFO_NUM_TYPES
 };
 
-- 
1.5.5

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

[Tristan Ye]

This new code is a bit more complicated than former ones, the goal is to
show user all statistics required to take a deep insight into filesystem
on how the disk is being fragmentaed.

The goal is achieved by scaning global bitmap from (cluster)group to group
to figure out following factors in the filesystem:

        - How many free chunks in a fixed size as user requested.
        - How many real free chunks in all size.
        - Min/Max/Avg size(in) clusters of free chunks.
        - How do free chunks distribute(in size) in terms of a histogram,
          just like following:
          ---------------------------------------------------------
          Extent Size Range :  Free extents  Free Clusters  Percent
             32K...   64K-  :             1             1    0.00%
              1M...    2M-  :             9           288    0.03%
              8M...   16M-  :             2           831    0.09%
             32M...   64M-  :             1          2047    0.23%
            128M...  256M-  :             1          8191    0.92%
            256M...  512M-  :             2         21706    2.43%
            512M... 1024M-  :            27        858623   96.29%
          ---------------------------------------------------------

Userspace ioctl() call eventually gets the above info returned by passing
a 'struct ocfs2_info_freefrag' with the chunk_size being specified first.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/ioctl.c       |  283 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/ocfs2_ioctl.h |   23 ++++
 2 files changed, 306 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index 18812be..9c5ee27 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -26,6 +26,7 @@
 #include "sysfile.h"
 #include "dir.h"
 #include "buffer_head_io.h"
+#include "suballoc.h"
 
 #include <linux/ext2_fs.h>
 
@@ -439,6 +440,284 @@ bail:
 	return status;
 }
 
+static void o2ffg_update_histogram(struct ocfs2_info_free_chunk_list *hist,
+				   unsigned int chunksize)
+{
+	int index;
+
+	index = __ilog2_u32(chunksize);
+	if (index >= OCFS2_INFO_MAX_HIST)
+		index = OCFS2_INFO_MAX_HIST - 1;
+
+	hist->fc_chunks[index]++;
+	hist->fc_clusters[index] += chunksize;
+}
+
+static void o2ffg_update_stats(struct ocfs2_info_freefrag_stats *stats,
+			       unsigned int chunksize)
+{
+	if (chunksize > stats->ffs_max)
+		stats->ffs_max = chunksize;
+
+	if (chunksize < stats->ffs_min)
+		stats->ffs_min = chunksize;
+
+	stats->ffs_avg += chunksize;
+	stats->ffs_free_chunks_real++;
+}
+
+void ocfs2_info_update_ffg(struct ocfs2_info_freefrag *ffg,
+			   unsigned int chunksize)
+{
+	o2ffg_update_histogram(&(ffg->iff_ffs.ffs_fc_hist), chunksize);
+	o2ffg_update_stats(&(ffg->iff_ffs), chunksize);
+}
+
+int ocfs2_info_freefrag_scan_chain(struct ocfs2_super *osb,
+				   struct inode *gb_inode,
+				   struct ocfs2_dinode *gb_dinode,
+				   struct ocfs2_chain_rec *rec,
+				   struct ocfs2_info_freefrag *ffg,
+				   u32 chunks_in_group)
+{
+	int status = 0, used;
+	u64 blkno;
+
+	struct buffer_head *bh = NULL;
+	struct ocfs2_group_desc *bg = NULL;
+
+	unsigned int max_bits, num_clusters;
+	unsigned int offset = 0, cluster, chunk;
+	unsigned int chunk_free, last_chunksize = 0;
+
+	if (!le32_to_cpu(rec->c_free))
+		goto bail;
+
+	do {
+		if (!bg)
+			blkno = le64_to_cpu(rec->c_blkno);
+		else
+			blkno = le64_to_cpu(bg->bg_next_group);
+
+		if (bh) {
+			brelse(bh);
+			bh = NULL;
+		}
+
+		if (o2info_coherent(*ffg))
+			status = ocfs2_read_group_descriptor(gb_inode,
+							     gb_dinode,
+							     blkno, &bh);
+		else
+			status = ocfs2_read_blocks_sync(osb, blkno, 1, &bh);
+
+		if (status < 0) {
+			mlog(ML_ERROR, "Can't read the group descriptor # "
+			     "%llu from device.", (unsigned long long)blkno);
+			status = -EIO;
+			goto bail;
+		}
+
+		bg = (struct ocfs2_group_desc *)bh->b_data;
+
+		if (!le16_to_cpu(bg->bg_free_bits_count))
+			continue;
+
+		max_bits = le16_to_cpu(bg->bg_bits);
+		offset = 0;
+
+		for (chunk = 0; chunk < chunks_in_group; chunk++) {
+			/*
+			 * last chunk may be not an entire one.
+			 */
+			if ((offset + ffg->iff_chunksize) > max_bits)
+				num_clusters = max_bits - offset;
+			else
+				num_clusters = ffg->iff_chunksize;
+
+			chunk_free = 0;
+			for (cluster = 0; cluster < num_clusters; cluster++) {
+				used = ocfs2_test_bit(offset,
+						(unsigned long *)bg->bg_bitmap);
+				/*
+				 * - chunk_free counts free clusters in #N chunk.
+				 * - last_chunksize records the size(in) clusters
+				 *   for the last real free chunk being counted.
+				 */
+				if (!used) {
+					last_chunksize++;
+					chunk_free++;
+				}
+
+				if (used && last_chunksize) {
+					ocfs2_info_update_ffg(ffg,
+							      last_chunksize);
+					last_chunksize = 0;
+				}
+
+				offset++;
+			}
+
+			if (chunk_free == ffg->iff_chunksize)
+				ffg->iff_ffs.ffs_free_chunks++;
+		}
+
+		/*
+		 * need to update the info for last free chunk.
+		 */
+		if (last_chunksize)
+			ocfs2_info_update_ffg(ffg, last_chunksize);
+
+	} while (le64_to_cpu(bg->bg_next_group));
+
+bail:
+	brelse(bh);
+
+	mlog_exit(status);
+	return status;
+}
+
+int ocfs2_info_freefrag_scan_bitmap(struct ocfs2_super *osb,
+				    struct inode *gb_inode, u64 blkno,
+				    struct ocfs2_info_freefrag *ffg)
+{
+	u32 chunks_in_group;
+	int status = 0, unlock = 0, i;
+
+	struct buffer_head *bh = NULL;
+	struct ocfs2_chain_list *cl = NULL;
+	struct ocfs2_chain_rec *rec = NULL;
+	struct ocfs2_dinode *gb_dinode = NULL;
+
+	if (gb_inode)
+		mutex_lock(&gb_inode->i_mutex);
+
+	if (o2info_coherent(*ffg)) {
+		status = ocfs2_inode_lock(gb_inode, &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;
+		}
+	}
+
+	gb_dinode = (struct ocfs2_dinode *)bh->b_data;
+	cl = &(gb_dinode->id2.i_chain);
+
+	/*
+	 * Chunksize(in) clusters from userspace should be
+	 * less than clusters in a group.
+	 */
+	if (ffg->iff_chunksize > le16_to_cpu(cl->cl_cpg)) {
+		status = -EINVAL;
+		goto bail;
+	}
+
+	memset(&ffg->iff_ffs, 0, sizeof(struct ocfs2_info_freefrag_stats));
+
+	ffg->iff_ffs.ffs_min = ~0U;
+	ffg->iff_ffs.ffs_clusters =
+			le32_to_cpu(gb_dinode->id1.bitmap1.i_total);
+	ffg->iff_ffs.ffs_free_clusters = ffg->iff_ffs.ffs_clusters -
+			le32_to_cpu(gb_dinode->id1.bitmap1.i_used);
+
+	chunks_in_group = le16_to_cpu(cl->cl_cpg) / ffg->iff_chunksize + 1;
+
+	for (i = 0; i < le16_to_cpu(cl->cl_next_free_rec); i++) {
+		rec = &(cl->cl_recs[i]);
+		status = ocfs2_info_freefrag_scan_chain(osb, gb_inode,
+							gb_dinode,
+							rec, ffg,
+							chunks_in_group);
+		if (status)
+			goto bail;
+	}
+
+	if (ffg->iff_ffs.ffs_free_chunks_real)
+		ffg->iff_ffs.ffs_avg = (ffg->iff_ffs.ffs_avg /
+					ffg->iff_ffs.ffs_free_chunks_real);
+bail:
+	if (unlock)
+		ocfs2_inode_unlock(gb_inode, 0);
+
+	if (gb_inode)
+		mutex_unlock(&gb_inode->i_mutex);
+
+	if (gb_inode)
+		iput(gb_inode);
+
+	brelse(bh);
+
+	mlog_exit(status);
+	return status;
+}
+
+int ocfs2_info_handle_freefrag(struct inode *inode,
+			       struct ocfs2_info_request __user *req)
+{
+	u64 blkno = -1;
+	char namebuf[40];
+	int status = -EFAULT, type = GLOBAL_BITMAP_SYSTEM_INODE;
+
+	struct ocfs2_info_freefrag oiff;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct inode *gb_inode = NULL;
+
+	if (o2info_from_user(oiff, req))
+		goto bail;
+	/*
+	 * chunksize from userspace should be power of 2.
+	 */
+	if ((oiff.iff_chunksize & (oiff.iff_chunksize - 1)) ||
+	    (!oiff.iff_chunksize)) {
+		status = -EINVAL;
+		goto bail;
+	}
+
+	if (o2info_coherent(oiff)) {
+		gb_inode = ocfs2_get_system_file_inode(osb, type,
+						       OCFS2_INVALID_SLOT);
+		if (!gb_inode) {
+			mlog(ML_ERROR, "unable to get global_bitmap inode\n");
+			status = -EIO;
+			goto bail;
+		}
+	} else {
+		ocfs2_sprintf_system_inode_name(namebuf, sizeof(namebuf), type,
+						OCFS2_INVALID_SLOT);
+		status = ocfs2_lookup_ino_from_name(osb->sys_root_inode,
+						    namebuf,
+						    strlen(namebuf),
+						    &blkno);
+		if (status < 0) {
+			status = -ENOENT;
+			goto bail;
+		}
+	}
+
+	status = ocfs2_info_freefrag_scan_bitmap(osb, gb_inode, blkno, &oiff);
+	if (status < 0)
+		goto bail;
+
+	o2info_set_request_filled(oiff);
+
+	if (o2info_to_user(oiff, req))
+		goto bail;
+
+	status = 0;
+bail:
+	if (status)
+		o2info_set_request_error(oiff, req);
+
+	return status;
+}
+
 int ocfs2_info_handle_unknown(struct inode *inode,
 			      struct ocfs2_info_request __user *req)
 {
@@ -514,6 +793,10 @@ int ocfs2_info_handle_request(struct inode *inode,
 		if (oir.ir_size == sizeof(struct ocfs2_info_freeinode))
 			status = ocfs2_info_handle_freeinode(inode, req);
 		break;
+	case OCFS2_INFO_FREEFRAG:
+		if (oir.ir_size == sizeof(struct ocfs2_info_freefrag))
+			status = ocfs2_info_handle_freefrag(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 6b4b39a..18b6770 100644
--- a/fs/ocfs2/ocfs2_ioctl.h
+++ b/fs/ocfs2/ocfs2_ioctl.h
@@ -152,6 +152,28 @@ struct ocfs2_info_freeinode {
 	__u32 ifi_pad;
 };
 
+#define OCFS2_INFO_MAX_HIST     (32)
+
+struct ocfs2_info_freefrag {
+	struct ocfs2_info_request iff_req;
+	struct ocfs2_info_freefrag_stats { /* (out) */
+		struct ocfs2_info_free_chunk_list {
+			__u32 fc_chunks[OCFS2_INFO_MAX_HIST];
+			__u32 fc_clusters[OCFS2_INFO_MAX_HIST];
+		} ffs_fc_hist;
+		__u32 ffs_clusters;
+		__u32 ffs_free_clusters;
+		__u32 ffs_free_chunks;
+		__u32 ffs_free_chunks_real;
+		__u32 ffs_min; /* Minimum free chunksize in clusters */
+		__u32 ffs_max;
+		__u32 ffs_avg;
+		__u32 ffs_pad;
+	} iff_ffs;
+	__u32 iff_chunksize; /* chunksize in clusters(in) */
+	__u32 iff_pad;
+};
+
 /* Codes for ocfs2_info_request */
 enum ocfs2_info_type {
 	OCFS2_INFO_CLUSTERSIZE = 1,
@@ -162,6 +184,7 @@ enum ocfs2_info_type {
 	OCFS2_INFO_FS_FEATURES,
 	OCFS2_INFO_JOURNAL_SIZE,
 	OCFS2_INFO_FREEINODE,
+	OCFS2_INFO_FREEFRAG,
 	OCFS2_INFO_NUM_TYPES
 };
 
-- 
1.5.5

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Mark Fasheh]

On Sun, Jan 30, 2011 at 02:25:59PM +0800, Tristan Ye wrote:
> It's a best-effort attempt to simplize duplicated codes here.

Nice cleanup. I have only one issue with it:


> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
> ---
>  fs/ocfs2/ioctl.c |   38 ++++++++++++++++++++++++++++++--------
>  1 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index 7a48681..731cf46 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -46,6 +46,22 @@ static inline void __o2info_set_request_error(struct ocfs2_info_request *kreq,
>  #define o2info_set_request_error(a, b) \
>  		__o2info_set_request_error((struct ocfs2_info_request *)&(a), b)
>  
> +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
> +{
> +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> +}
> +
> +#define o2info_set_request_filled(a) \
> +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))

The macro here (and below) casts it's argument, thus defeating any
typechecking we would have gotten from the function call. Can you
pleaseremove the macro, rename the functions (take out the __) and use them
directly? I know we might then want to deref the i*_req field in the
handlers below but I don't think that's a big deal for what we gain.
	--Mark


--
Mark Fasheh

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

[Mark Fasheh]

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

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Joel Becker]

On Mon, Jan 31, 2011 at 02:15:47PM -0800, Mark Fasheh wrote:
> > +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
> > +{
> > +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> > +}
> > +
> > +#define o2info_set_request_filled(a) \
> > +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
> 
> The macro here (and below) casts it's argument, thus defeating any
> typechecking we would have gotten from the function call. Can you
> pleaseremove the macro, rename the functions (take out the __) and use them
> directly? I know we might then want to deref the i*_req field in the
> handlers below but I don't think that's a big deal for what we gain.

	I'm not sure what you mean here.  Are you asking for one
set_request_filled() call per info type?

---------------------------------------------
static inline void o2info_clustersize_set_request_filled(struct ocfs2_info_clutersize *ic)
{
	ic->ic_req.ir_flags |= OCFS2_INFO_FL_FILLED;
}

static inline void o2info_blocksize_set_request_filled(struct ocfs2_info_blocksize *ib)
{
	ib->ib_req.ir_flags |= OCFS2_INFO_FL_FILLED;
}

...

---------------------------------------------

	Because that is way uglier than open-coding the |= in the
functions, IMHO.

Joel

-- 

"One of the symptoms of an approaching nervous breakdown is the
 belief that one's work is terribly important."
         - Bertrand Russell 

			http://www.jlbec.org/
			jlbec at evilplan.org

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Mark Fasheh]

On Mon, Jan 31, 2011 at 05:10:12PM -0800, Joel Becker wrote:
> On Mon, Jan 31, 2011 at 02:15:47PM -0800, Mark Fasheh wrote:
> > > +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
> > > +{
> > > +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> > > +}
> > > +
> > > +#define o2info_set_request_filled(a) \
> > > +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
> > 
> > The macro here (and below) casts it's argument, thus defeating any
> > typechecking we would have gotten from the function call. Can you
> > pleaseremove the macro, rename the functions (take out the __) and use them
> > directly? I know we might then want to deref the i*_req field in the
> > handlers below but I don't think that's a big deal for what we gain.
> 
> 	I'm not sure what you mean here.  Are you asking for one
> set_request_filled() call per info type?

Oh, no!

I was asking for this:

static inline void o2info_set_request_filled(struct ocfs2_info_request *req)
{
	req->ir_flags |= OCFS2_INFO_FL_FILLED;
}

instead of the macro which is defeating type checking.

Hope that's more clear.
	--Mark

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Joel Becker]

On Mon, Jan 31, 2011 at 07:06:18PM -0800, Mark Fasheh wrote:
> On Mon, Jan 31, 2011 at 05:10:12PM -0800, Joel Becker wrote:
> > On Mon, Jan 31, 2011 at 02:15:47PM -0800, Mark Fasheh wrote:
> > > > +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
> > > > +{
> > > > +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> > > > +}
> > > > +
> > > > +#define o2info_set_request_filled(a) \
> > > > +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
> > > 
> > > The macro here (and below) casts it's argument, thus defeating any
> > > typechecking we would have gotten from the function call. Can you
> > > pleaseremove the macro, rename the functions (take out the __) and use them
> > > directly? I know we might then want to deref the i*_req field in the
> > > handlers below but I don't think that's a big deal for what we gain.
> > 
> > 	I'm not sure what you mean here.  Are you asking for one
> > set_request_filled() call per info type?
> 
> Oh, no!
> 
> I was asking for this:
> 
> static inline void o2info_set_request_filled(struct ocfs2_info_request *req)
> {
> 	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> }
> 
> instead of the macro which is defeating type checking.

	You want the casts or derefs forced in the caller, then?

	o2info_set_request_filled((struct ocfs2_info_request *)ic);

or:

	o2info_set_request_filled(&ic->ic_req);

Joel

-- 

Life's Little Instruction Book #80

	"Slow dance"

			http://www.jlbec.org/
			jlbec at evilplan.org

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Tristan Ye]

Mark Fasheh wrote:
> On Sun, Jan 30, 2011 at 02:25:59PM +0800, Tristan Ye wrote:
>> It's a best-effort attempt to simplize duplicated codes here.
>
> Nice cleanup. I have only one issue with it:
>
>
>> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
>> ---
>>  fs/ocfs2/ioctl.c |   38 ++++++++++++++++++++++++++++++--------
>>  1 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
>> index 7a48681..731cf46 100644
>> --- a/fs/ocfs2/ioctl.c
>> +++ b/fs/ocfs2/ioctl.c
>> @@ -46,6 +46,22 @@ static inline void __o2info_set_request_error(struct ocfs2_info_request *kreq,
>>  #define o2info_set_request_error(a, b) \
>>  		__o2info_set_request_error((struct ocfs2_info_request *)&(a), b)
>>  
>> +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
>> +{
>> +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +}
>> +
>> +#define o2info_set_request_filled(a) \
>> +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
>
> The macro here (and below) casts it's argument, thus defeating any
> typechecking we would have gotten from the function call. Can you
> pleaseremove the macro, rename the functions (take out the __) and use them
> directly? I know we might then want to deref the i*_req field in the
> handlers below but I don't think that's a big deal for what we gain.

Hi Mark,

    The reason why I used a macro is to make the life of caller as easy 
as taking the
specific info object(like ocfs2_info_blocksize, or 
ocfs2_info_clustersize etc) directly,
just for the sake of making the codes look more neat.

    That's not a big deal to use a direct inline func, while need caller 
bother to cast
the info type;-)

Tristan.

> 	--Mark
>
>
> --
> Mark Fasheh

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

[Tristan Ye]

Mark Fasheh wrote:
> 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.

Good catch, better to iput the inode in the same place where it was got.

>
>> +
>> +	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];
> };

    Yep, placing the struct in the end didn't hurt any alignment 
problem, while I'm
wondering how it will be hurting when It wasn't;-)




> 	--Mark
>
> --
> Mark Fasheh

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Tristan Ye]

Joel Becker wrote:
> On Mon, Jan 31, 2011 at 07:06:18PM -0800, Mark Fasheh wrote:
>> On Mon, Jan 31, 2011 at 05:10:12PM -0800, Joel Becker wrote:
>>> On Mon, Jan 31, 2011 at 02:15:47PM -0800, Mark Fasheh wrote:
>>>>> +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
>>>>> +{
>>>>> +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
>>>>> +}
>>>>> +
>>>>> +#define o2info_set_request_filled(a) \
>>>>> +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
>>>> The macro here (and below) casts it's argument, thus defeating any
>>>> typechecking we would have gotten from the function call. Can you
>>>> pleaseremove the macro, rename the functions (take out the __) and use them
>>>> directly? I know we might then want to deref the i*_req field in the
>>>> handlers below but I don't think that's a big deal for what we gain.
>>> 	I'm not sure what you mean here.  Are you asking for one
>>> set_request_filled() call per info type?
>> Oh, no!
>>
>> I was asking for this:
>>
>> static inline void o2info_set_request_filled(struct ocfs2_info_request *req)
>> {
>> 	req->ir_flags |= OCFS2_INFO_FL_FILLED;
>> }
>>
>> instead of the macro which is defeating type checking.
>
> 	You want the casts or derefs forced in the caller, then?
>
> 	o2info_set_request_filled((struct ocfs2_info_request *)ic);
>
> or:
>
> 	o2info_set_request_filled(&ic->ic_req);
Joel,

    Indeed! and seems it will not be making caller's life that hard;-)


   
   
>
> Joel
>

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Mark Fasheh]

On Mon, Jan 31, 2011 at 10:01:27PM -0800, Joel Becker wrote:
> On Mon, Jan 31, 2011 at 07:06:18PM -0800, Mark Fasheh wrote:
> > On Mon, Jan 31, 2011 at 05:10:12PM -0800, Joel Becker wrote:
> > > On Mon, Jan 31, 2011 at 02:15:47PM -0800, Mark Fasheh wrote:
> > > > > +static inline void __o2info_set_request_filled(struct ocfs2_info_request *req)
> > > > > +{
> > > > > +	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> > > > > +}
> > > > > +
> > > > > +#define o2info_set_request_filled(a) \
> > > > > +		__o2info_set_request_filled((struct ocfs2_info_request *)&(a))
> > > > 
> > > > The macro here (and below) casts it's argument, thus defeating any
> > > > typechecking we would have gotten from the function call. Can you
> > > > pleaseremove the macro, rename the functions (take out the __) and use them
> > > > directly? I know we might then want to deref the i*_req field in the
> > > > handlers below but I don't think that's a big deal for what we gain.
> > > 
> > > 	I'm not sure what you mean here.  Are you asking for one
> > > set_request_filled() call per info type?
> > 
> > Oh, no!
> > 
> > I was asking for this:
> > 
> > static inline void o2info_set_request_filled(struct ocfs2_info_request *req)
> > {
> > 	req->ir_flags |= OCFS2_INFO_FL_FILLED;
> > }
> > 
> > instead of the macro which is defeating type checking.
> 
> 	You want the casts or derefs forced in the caller, then?
> 
> 	o2info_set_request_filled((struct ocfs2_info_request *)ic);
> 
> or:
> 
> 	o2info_set_request_filled(&ic->ic_req);

Derefs in the callers would make most sense if we want to keep the type
checking in tact.
	--Mark

--
Mark Fasheh

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

[Joel Becker]

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))
> +
>  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);
> +
> +	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;

/build/jlbec/linux-2.6/working/fs/ocfs2/ioctl.c: In function ?ocfs2_info_handle_freeinode?:
/build/jlbec/linux-2.6/working/fs/ocfs2/ioctl.c:441: warning: the frame size of 4192 bytes is larger than 2048 bytes

	This is speaking to the fact that you've put struct
ocfs2_info_freeinode oifi on the stack.  It's over 4K in size.  It needs
to be allocated.
	Please respin this and the FREEFRAG patch based on it.

Joel

-- 

Life's Little Instruction Book #43

	"Never give up on somebody.  Miracles happen every day."

			http://www.jlbec.org/
			jlbec at evilplan.org

[Ocfs2-devel] [PATCH 1/3] Ocfs2: Using macro to set/clear *FILLED* flags in info handler.

[Joel Becker]

On Sun, Jan 30, 2011 at 02:25:59PM +0800, Tristan Ye wrote:
> It's a best-effort attempt to simplize duplicated codes here.
> 
> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>

	This patch is now in the merge-window branch of ocfs2.git.

Joel

-- 

"Hell is oneself, hell is alone, the other figures in it, merely projections."
        - T. S. Eliot

			http://www.jlbec.org/
			jlbec at evilplan.org

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

[Tristan Ye]

Joel Becker wrote:
> 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))
>> +
>>  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);
>> +
>> +	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;
>
> /build/jlbec/linux-2.6/working/fs/ocfs2/ioctl.c: In function ?ocfs2_info_handle_freeinode?:
> /build/jlbec/linux-2.6/working/fs/ocfs2/ioctl.c:441: warning: the frame size of 4192 bytes is larger than 2048 bytes
>
> 	This is speaking to the fact that you've put struct
> ocfs2_info_freeinode oifi on the stack.  It's over 4K in size.  It needs
> to be allocated.

    Joel, great catch, how did you builder get warning like that, needs 
to change makefile a bit?

And we're not allowed to put structure more than 2k, on stack for each 
function in kernel?


> 	Please respin this and the FREEFRAG patch based on it.
>
> Joel
>

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

[Joel Becker]

On Sun, Feb 20, 2011 at 08:59:51PM +0800, Tristan Ye wrote:
> > /build/jlbec/linux-2.6/working/fs/ocfs2/ioctl.c: In function ?ocfs2_info_handle_freeinode?:
> > /build/jlbec/linux-2.6/working/fs/ocfs2/ioctl.c:441: warning: the frame size of 4192 bytes is larger than 2048 bytes
> >
> > 	This is speaking to the fact that you've put struct
> > ocfs2_info_freeinode oifi on the stack.  It's over 4K in size.  It needs
> > to be allocated.
> 
>     Joel, great catch, how did you builder get warning like that, needs 
> to change makefile a bit?

	I didn't change anything.  My gcc (4.4.3 on amd64) noticed it.
I can't think of anything terribly odd about my .config.

> And we're not allowed to put structure more than 2k, on stack for each 
> function in kernel?

	We shouldn't put anything remotely close to that on the stack.
Ever.  Imagine a function with a 2K object calling another function with
a 2K object.  Bammo!  If you have anything larger than probably 256B on
the stack, you should know, precisely, what it calls, and that it can't
take a deep interrupt.

Joel

-- 

"I'm so tired of being tired,
 Sure as night will follow day.
 Most things I worry about
 Never happen anyway."

			http://www.jlbec.org/
			jlbec at evilplan.org

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

[Sunil Mushran]

On 02/20/2011 04:59 AM, Tristan Ye wrote:
>      Joel, great catch, how did you builder get warning like that, needs
> to change makefile a bit?
>
> And we're not allowed to put structure more than 2k, on stack for each
> function in kernel?

In 32-bit kernels, the stack is set to 4K. In 64 bit, 8K. These are the
typical values chosen by various distros.

So using 2K is dangerous as it could easily lead to an overflow and oops/panic
the box.

There is no magic cut-off value for the max stack footprint for a function.
The number depends on the stack depth. Also we have no control on a lot of
functions in the stack. So we try to keep our footprint as small as possible.

BTW, you can run scripts/checkstack.pl to check the stack footprint of the
functions.

# objdump -d fs/ocfs2/ocfs2.ko | scripts/checkstack.pl
0x0001873a ocfs2_extend_dir [ocfs2]:        316
0x00044060 ocfs2_rename [ocfs2]:            316
0x0003b5af __ocfs2_recovery_thread [ocfs2]: 232
0x00046cd8 ocfs2_symlink [ocfs2]:           220
0x000762f6 ocfs2_xattr_set [ocfs2]:         220

(So 316 bytes is the current max footprint for a function in 32-bit builds.)

Sunil

[Ocfs2-devel] [PATCH 0/3] Ocfs2: Adding new codes 'OCFS2_INFO_FREEINODE' and 'OCFS2_INFO_FREEFRAG' for o2info ioctl V2.

[Tristan Ye]

This set of patch series includes three patches:

1. Using marco to simplize duplicated codes.

2. Adding new code 'OCFS2_INODE_FREEINODE'

3. Adding new code 'OCFS2_INFO_FREEFRAG'

Changes since v1:

1. Incorporate Joel's comments to take cluster coherency into account when
   lookuping the inode/inode_blkno.

2. Using helpers somewhere to improve the readability of codes.



Tristan.