Hello, Andreas
On Saturday 15 March 2008 07:23:16 you wrote:
> On Mar 14, 2008 16:08 +0300, Andrew Perepechko wrote:
> > @@ -395,7 +463,10 @@ static int v2_write_dquot(struct dquot *
> > {
> > int type = dquot->dq_type;
> > ssize_t ret;
> > - struct v2_disk_dqblk ddquot, empty;
> > + union v2_disk_dqblk ddquot, empty;
> > + struct super_block *sb = dquot->dq_sb;
> > + uint rev = sb_dqopt(sb)->info[type].u.v2_i.dqi_revision;
> > + uint dqblksz = v2_dqblksz(rev);
>
> In a few places you add new on-stack variables like "sb", but they aren't
> used more than 1 or 2 times. While it makes the code a tiny bit clearer
> (though not largely so for "sb" because it is only dquot->dq_sb) it does
> increase the stack usage, and that is never a good idea.
I agree. Probably, it makes sense to rollback sb change. Though this may lead
to additional line breaks.
>
> > + union v2_disk_dqblk fakedquot;
> >
> > + memset(&fakedquot, 0, dqblksz);
> > + for (i = 0; i < dqstrinblk; i++)
> > + if (!DQF_GET(ddquot+i, rev, dqb_id) &&
> > + memcmp(&fakedquot, ddquot+i, dqblksz))
>
> Hmm, allocating "fakedquot" on the stack just to compare it to zero
> doesn't seem like a good use of space. What about doing the memcmp()
> against page_address(ZERO_PAGE(0))? It might be nice to have a permanent
> mapping of ZERO_PAGE(0) like void *zero_buffer that can be used for this.
>
What do you think about something like this?
--- quota_v2.c.saved 2008-03-14 00:07:04.000000000 +0300
+++ quota_v2.c 2008-03-15 16:17:04.000000000 +0300
@@ -26,6 +26,12 @@ typedef char *dqbuf_t;
#define GETENTRIES(buf) ((union v2_disk_dqblk *)(((char *)buf) + \
sizeof(struct v2_disk_dqdbheader)))
+static union v2_disk_dqblk emptydquot;
+static union v2_disk_dqblk fakedquot[2] = {
+ {.disk_dqblk_r0 = {.dqb_itime = __constant_cpu_to_le64(1LLU)}},
+ {.disk_dqblk_r1 = {.dqb_itime = __constant_cpu_to_le64(1LLU)}}
+};
+
static inline uint v2_dqblksz(uint rev)
{
uint sz = 0; /* make the compiler happy */
@@ -339,7 +345,6 @@ static uint find_free_dqentry(struct dqu
dqblksz = v2_dqblksz(rev), dqstrinblk = v2_dqstrinblk(rev);
struct v2_disk_dqdbheader *dh;
union v2_disk_dqblk *ddquot;
- union v2_disk_dqblk fakedquot;
dqbuf_t buf;
*err = 0;
@@ -373,9 +378,8 @@ static uint find_free_dqentry(struct dqu
goto out_buf;
}
dh->dqdh_entries = cpu_to_le16(le16_to_cpu(dh->dqdh_entries)+1);
- memset(&fakedquot, 0, dqblksz);
/* Find free structure in block */
- for (i = 0; i < dqstrinblk && memcmp(&fakedquot,
+ for (i = 0; i < dqstrinblk && memcmp(&emptydquot,
ddquot+i, dqblksz); i++);
#ifdef __QUOTA_V2_PARANOIA
if (i == dqstrinblk) {
@@ -463,7 +467,7 @@ static int v2_write_dquot(struct dquot *
{
int type = dquot->dq_type;
ssize_t ret;
- union v2_disk_dqblk ddquot, empty;
+ union v2_disk_dqblk ddquot;
struct super_block *sb = dquot->dq_sb;
uint rev = sb_dqopt(sb)->info[type].u.v2_i.dqi_revision;
uint dqblksz = v2_dqblksz(rev);
@@ -479,8 +483,7 @@ static int v2_write_dquot(struct dquot *
/* Argh... We may need to write structure full of zeroes but that would be
* treated as an empty place by the rest of the code. Format change would
* be definitely cleaner but the problems probably are not worth it */
- memset(&empty, 0, dqblksz);
- if (!memcmp(&empty, &ddquot, dqblksz))
+ if (!memcmp(&emptydquot, &ddquot, dqblksz))
DQF_PUT(&ddquot, rev, dqb_itime, 1);
spin_unlock(&dq_data_lock);
ret = sb->s_op->quota_write(sb, type,
@@ -629,12 +632,9 @@ static loff_t find_block_dqentry(struct
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... */
- union v2_disk_dqblk fakedquot;
-
- memset(&fakedquot, 0, dqblksz);
for (i = 0; i < dqstrinblk; i++)
if (!DQF_GET(ddquot+i, rev, dqb_id) &&
- memcmp(&fakedquot, ddquot+i, dqblksz))
+ memcmp(&emptydquot, ddquot+i, dqblksz))
break;
}
if (i == dqstrinblk) {
@@ -687,7 +687,7 @@ static int v2_read_dquot(struct dquot *d
{
int type = dquot->dq_type;
loff_t offset;
- union v2_disk_dqblk ddquot, empty;
+ union v2_disk_dqblk ddquot;
int ret = 0;
struct super_block *sb = dquot->dq_sb;
@@ -724,9 +724,7 @@ static int v2_read_dquot(struct dquot *d
else {
ret = 0;
/* We need to escape back all-zero structure */
- memset(&empty, 0, dqblksz);
- DQF_PUT(&empty, rev, dqb_itime, 1);
- if (!memcmp(&empty, &ddquot, dqblksz))
+ if (!memcmp(&fakedquot[rev], &ddquot, dqblksz))
DQF_PUT(&ddquot, rev, dqb_itime, 0);
}
disk2memdqb(&dquot->dq_dqb, &ddquot, rev);
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
