Hello, Andreas
On Sunday 16 March 2008 01:47:26 Andreas Dilger wrote:
>
> Is this ever used on values that are read from the disk? If yes, then
> having it BUG() because there is corruption on the disk is bad.
This is used on revision variable that is either 0 or 1, which is
determined by the special function. As this special function cannot
return any other values, having rev!=0&&rev!=1 just means we got
a memory corruption.
> This is no good - if there is an error from ->quota_read() then the kernel
> will BUG() here. You need proper (non fatal) error handling when you are
> first loading the quota information into memory.
My point is that something very very nasty have happened if we succeed on first
version check and fail right after that. In principle, we dont even need this
second check as we have done the check at this point. But using
results from previous check would need some interface changes(sigh).
I'll change the BUG_ON to returning error, but my point is if it happens, then
user is not expecting any sane error handling or it just won't help him. :)
>
> This could be inline.
>
I agree.
>
> While this produces nice looking code, it isn't very efficient to run.
> It has 8 checks for "rev" per call, and it would have an additional
> 8 checks for "sizeof" except those are constant and optimized away
> by the compiler. If you can convince me that the compiler will also
> optimize the 8 checks for "rev" into a single branch then it can
> stay, otherwise you should split it into 2 large sections based on
> "rev" and then decode r0 and r1 structures separately.
>
I'll check the code produced by gcc.
> > @@ -301,17 +371,18 @@ static uint find_free_dqentry(struct dqu
> > + /* Block will be full? */
> > + if (le16_to_cpu(dh->dqdh_entries)+1 >= dqstrinblk)
> > if ((*err = remove_free_dqentry(sb, dquot->dq_type, buf, blk)) < 0) {
> > printk(KERN_ERR "VFS: find_free_dqentry(): Can't remove block (%u) from entry free list.\n", blk);
> > goto out_buf;
> > }
>
> Can you please clean this up a bit while here:
>
> /* Block will be full? */
> if (le16_to_cpu(dh->dqdh_entries) + 1 >= dqstrinblk &&
> (*err = remove_free_dqentry(sb, dquot->dq_type, buf, blk)) < 0) {
> printk(KERN_ERR "VFS: find_free_dqentry(): Can't remove "
> "block (%u) from entry free list.\n", blk);
> goto out_buf;
> }
While this makes sense to me, this change would be prohibited by
checkpatch.pl because of adding (*err = remove_free_dqentry(sb, dquot->dq_type, buf, blk)
assignment in condition. Indeed, we can move further with the change, but
code refactoring/cleaning is not the purpose of my patch. :)
>
> > + for (i = 0; i < dqstrinblk && memcmp(&emptydquot,
> > + ddquot+i, dqblksz); i++);
>
> This is also quite easy to be confused, please write it as:
>
> for (i = 0; i < dqstrinblk && memcmp(&emptydquot,ddquot+i,dqblksz); i++)
> /* empty loop to count used dquot structs in block */;
You killed two spaces to fit the line in 80 chars. IIRC, checkpatch.pl requires that
args should be separated with spaces.
>
> In fact, it would be even better to just increment "ddquot" in the loop to
> avoid having to re-do the offset each time:
>
> for (i = 0; i < dqstrinblk && memcmp(&emptydquot, ddquot, dqblksz);
> i++, ddquot++)
> /* empty loop to count used dquot structs in block */;
>
> > - dquot->dq_off = (blk<<V2_DQBLKSIZE_BITS)+sizeof(struct v2_disk_dqdbheader)+i*sizeof(struct v2_disk_dqblk);
> > + dquot->dq_off = (blk<<V2_DQBLKSIZE_BITS)+
> > + sizeof(struct v2_disk_dqdbheader)+i*dqblksz;
>
> With the above change to ddquot you can then use it directly here:
>
> dquot->dq_off = (blk << V2_DQBLKSIZE_BITS) +
> ((char *)ddquot - (char *)buf);
I agree.
>
> > @@ -395,7 +467,9 @@ static int v2_write_dquot(struct dquot *
> > {
> > int type = dquot->dq_type;
>
> type is only used twice in this function, please remove it from the stack.
>
> > @@ -538,27 +613,29 @@ static loff_t find_block_dqentry(struct
> > + union v2_disk_dqblk *ddquot = GETENTRIES(buf);
> > + int type = dquot->dq_type;
>
> type only used twice in this function, please remove from stack.
>
> > + uint rev = sb_dqopt(dquot->dq_sb)->info[type].u.v2_i.dqi_revision;
> > + uint dqblksz = v2_dqblksz(rev), dqstrinblk = v2_dqstrinblk(rev);
> >
> > if (dquot->dq_id)
> > - for (i = 0; i < V2_DQSTRINBLK &&
> > - le32_to_cpu(ddquot[i].dqb_id) != dquot->dq_id; i++);
> > + for (i = 0; i < dqstrinblk &&
> > + DQF_GET(ddquot+i, rev, dqb_id) != dquot->dq_id; i++);
> > else { /* ID 0 as a bit more complicated searching... */
>
> Spaces around " + ", and please add a { } around this block because second
> part has { } already.
>
> > + for (i = 0; i < dqstrinblk; i++)
> > + if (!DQF_GET(ddquot+i, rev, dqb_id) &&
> > + memcmp(&emptydquot, ddquot+i, dqblksz))
> > break;
> > }
>
> Spaces around " + " here also. Actually, please run the patch through
> "checkpatch.pl" to find any similar issues.
>
> > @@ -608,7 +685,7 @@ static int v2_read_dquot(struct dquot *d
> > {
> > int type = dquot->dq_type;
> > loff_t offset;
> > - struct v2_disk_dqblk ddquot, empty;
> > + union v2_disk_dqblk ddquot;
>
> Please remove type from stack.
>
These comments refer mostly to enhancements to the original code. While I agree with most of them,
I see no reason, why they should be a part of this very patch. As for checkpatch.pl, I did run
it before emailing the patch and it did not find absent spaces around "+" to be an issue:
panda:/usr/src/linux-2.6.24.3 # md5sum /usr/src/quota.patch
e306aaf884b11709d9e744dfa5e89672 /usr/src/quota.patch
panda:/usr/src/linux-2.6.24.3 # scripts/checkpatch.pl /usr/src/quota.patch
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 460 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
> > @@ -629,25 +706,25 @@ static int v2_read_dquot(struct dquot *d
> > + uint rev = sb_dqopt(dquot->dq_sb)->info[type].u.v2_i.
> > + dqi_revision, dqblksz = v2_dqblksz(rev);
>
> Use another line with "uint dqblksz = v2_dqblksz(rev);" here.
>
I agree.
> > struct mem_dqblk {
> > - __u32 dqb_bhardlimit; /* absolute limit on disk blks alloc */
> > - __u32 dqb_bsoftlimit; /* preferred limit on disk blks */
> > + qsize_t dqb_bhardlimit; /* absolute limit on disk blks alloc */
> > + qsize_t dqb_bsoftlimit; /* preferred limit on disk blks */
> > qsize_t dqb_curspace; /* current used space */
> > - __u32 dqb_ihardlimit; /* absolute limit on allocated inodes */
> > - __u32 dqb_isoftlimit; /* preferred inode limit */
> > - __u32 dqb_curinodes; /* current # allocated inodes */
> > + qsize_t dqb_ihardlimit; /* absolute limit on allocated inodes */
> > + qsize_t dqb_isoftlimit; /* preferred inode limit */
> > + qsize_t dqb_curinodes; /* current # allocated inodes */
> > time_t dqb_btime; /* time limit for excessive disk use */
> > time_t dqb_itime; /* time limit for excessive inode use */
> > };
>
> Is there a reason to use "qsize_t" instead of just using __u64 directly?
>
Well, I'm not sure if there's a special meaning for qsize_t. I have only followed
the dqb_curspace convention, which is of qsize_t type. While I see no
special usage for qsize_t (well, it looks more nice than __u64 :), I at the same time
see no problems using qsize_t. Maybe Jan will tell us more about qsize_t.
> > +++ linux-2.6.24.3/include/linux/quotaio_v2.h 2008-03-13 22:01:48.000000000 +0300
> > @@ -16,28 +16,51 @@
> > @@ -59,7 +82,7 @@ struct v2_disk_dqinfo {
> >
> > /*
> > * Structure of header of block with quota structures. It is padded to 16 bytes so
> > - * there will be space for exactly 21 quota-entries in a block
> > + * there will be space for exactly 21 (r0) or 14 (r1) quota-entries in a block
> > */
>
> Hmm, when this comment says "block" does it mean "1024-byte block" or
> filesystem block? For 1024-byte block it is true that both 48-byte (r0) and
> 72-byte (r1) records fit evenly, but for 4096-byte blocks 72-byte records
> do not fit evenly. Is that a problem?
Quota file is internally divided into 1024 byte blocks (see V2_DQBLKSIZE constant) each
consisting of a block header (which is used for free space management) and a fixed number
of quota entries. This is not related in any way to fs blocks. quota format module
interacts with underlying storage only through quota_read/quota_write interface.
I'm going to update the patch with respect to your comments.
Btw, I lost "static" before disk2memdqblk, I'm going to update this one too.
Thanks and cheers.
Andrew.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
