From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Becker Date: Tue, 21 Oct 2008 16:10:46 -0700 Subject: [Ocfs2-devel] [PATCH] ocfs2: Implementation of local and global quota file handling In-Reply-To: <12245234431180-git-send-email-jack@suse.cz> References: <12245234431180-git-send-email-jack@suse.cz> Message-ID: <20081021231046.GH2871@mail.oracle.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ocfs2-devel@oss.oracle.com On Mon, Oct 20, 2008 at 07:23:57PM +0200, Jan Kara wrote: > Signed-off-by: Jan Kara The commit message could maybe use a little discussion of what quota bits go in the local file and what goes in the global. It seems like less is written to the global file, but I might be wrong about that (eg, generic quota code might be writing there). > diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h > new file mode 100644 > index 0000000..33b47eb > --- /dev/null > +++ b/fs/ocfs2/quota.h > @@ -0,0 +1,182 @@ > +/* > + * quota.h for OCFS2 > + * > + * On disk quota structures for local and global quota file, in-memory > + * structures. I'm thinking the on-disk structures belong in ocfs2_fs.h, which is where we define the disk format, and the in-memory structures belong here. > +/* Information header of local quota file (immediately follows the generic > + * header) */ > +struct ocfs2_local_disk_dqinfo { > + __le32 dqi_flags; /* Flags for quota file */ > + __le32 dqi_chunks; /* Number of chunks of quota structures > + * with a bitmap */ > + __le32 dqi_blocks; /* Number of blocks allocated for quota file */ > +}; Also, we'd love the on-disk structures to have the size/offset annotations we put in ocfs2_fs.h. Yes, some of the smaller structures are pretty obvious, but having the hexadecimal values in the header often helps reading binary dumps. > + while (toread > 0) { > + tocopy = min(sb->s_blocksize - offset, toread); > + bh = ocfs2_bread(gqinode, blk, &err, 0); > + if (!bh) { > + mlog_errno(err); > + return err; > + } Fair warning - you use ocfs2_bread() a lot in here, but ocfs2_bread() is going away. Before I realized you were going to use it (and conceptually that makes sense), it moved into fs/ocfs2/dir.c. Then it morphed into ocfs2_read_dir_block()as part of my read-metadata branch at http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=shortlog;h=read-metadata In the end, this code will be doing ocfs2_read_quota_block(), or perhaps different variations thereof (if there are different types of blocks in the quota file). I'm going to refactor ocfs2_read_dir_block() and introduce the function ocfs2_read_virt_blocks(). It will look exactly like ocfs2_read_blocks() does in my read-metadata branch, except that it will take virtual block numbers rather than physical ones. Thus, you'll be able to write ocfs2_read_quota_block() as a thin wrapper around it, and replace your ocfs2_bread() calls easily. Obviously, you don't have to worry too much until this code moves atop the latest kernel code. And I'm happy to help. > +/* Write bufferhead into the fs */ > +static int ocfs2_modify_bh(struct inode *inode, struct buffer_head *bh, > + void (*modify)(struct buffer_head *, void *), void *private) > +{ > + struct super_block *sb = inode->i_sb; > + handle_t *handle; > + int status; > + > + handle = ocfs2_start_trans(OCFS2_SB(sb), 1); > + if (IS_ERR(handle)) { > + status = PTR_ERR(handle); > + mlog_errno(status); > + return status; > + } > + status = ocfs2_journal_access(handle, inode, bh, > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + ocfs2_commit_trans(OCFS2_SB(sb), handle); > + return status; > + } > + lock_buffer(bh); > + modify(bh, private); > + unlock_buffer(bh); > + status = ocfs2_journal_dirty(handle, bh); > + if (status < 0) { > + mlog_errno(status); > + ocfs2_commit_trans(OCFS2_SB(sb), handle); > + return status; > + } ocfs2_journal_dirty() can't sanely return an error, because journal_dirty() can't either. Someday it will become void. You can take out all the error checking of your ocfs2_journal_dirty() calls. Is this modify_bh() function the reason you need nested transactions? The callers can't be audited for their transaction state? Joel -- "It is not the function of our government to keep the citizen from falling into error; it is the function of the citizen to keep the government from falling into error." - Robert H. Jackson Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127