AFFS code preallocates several blocks as an optimisation. Unfortunately
it's not protected by lock so the same blocks may end up allocated twice.
Here is a fix.
Signed-off-by: Vladimir Serbinenko <[email protected]>
diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index 45a0ce4..fc1d4ca 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -66,6 +66,8 @@ struct affs_inode_info {
u32 i_protect; /* unused attribute bits */
u32 i_lastalloc; /* last allocated block */
int i_pa_cnt; /* number of preallocated blocks */
+ spinlock_t i_alloc; /* Protects last 2 fields. */
+
struct inode vfs_inode;
};
diff --git a/fs/affs/bitmap.c b/fs/affs/bitmap.c
index 3e26271..3341bde 100644
--- a/fs/affs/bitmap.c
+++ b/fs/affs/bitmap.c
@@ -151,12 +151,18 @@ affs_alloc_block(struct inode *inode, u32 goal)
pr_debug("AFFS: balloc(inode=%lu,goal=%u): ", inode->i_ino, goal);
+ spin_lock(&AFFS_I(inode)->i_alloc);
+
if (AFFS_I(inode)->i_pa_cnt) {
- pr_debug("%d\n", AFFS_I(inode)->i_lastalloc+1);
+ u32 ret;
AFFS_I(inode)->i_pa_cnt--;
- return ++AFFS_I(inode)->i_lastalloc;
+ ret = ++AFFS_I(inode)->i_lastalloc;
+ spin_unlock(&AFFS_I(inode)->i_alloc);
+ return ret;
}
+ spin_unlock(&AFFS_I(inode)->i_alloc);
+
if (!goal || goal > sbi->s_partition_size) {
if (goal)
affs_warning(sb, "affs_balloc", "invalid goal %d", goal);
@@ -230,16 +236,22 @@ find_bit:
bit = ffs(tmp & mask) - 1;
blk += bit + sbi->s_reserved;
mask2 = mask = 1 << (bit & 31);
- AFFS_I(inode)->i_lastalloc = blk;
-
- /* prealloc as much as possible within this word */
- while ((mask2 <<= 1)) {
- if (!(tmp & mask2))
- break;
- AFFS_I(inode)->i_pa_cnt++;
- mask |= mask2;
+
+ spin_lock(&AFFS_I(inode)->i_alloc);
+ if (!AFFS_I(inode)->i_pa_cnt) {
+ AFFS_I(inode)->i_lastalloc = blk;
+
+ /* prealloc as much as possible within this word */
+ while ((mask2 <<= 1)) {
+ if (!(tmp & mask2))
+ break;
+ AFFS_I(inode)->i_pa_cnt++;
+ mask |= mask2;
+ }
+ bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
}
- bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
+
+ spin_unlock(&AFFS_I(inode)->i_alloc);
*data = cpu_to_be32(tmp & ~mask);
diff --git a/fs/affs/file.c b/fs/affs/file.c
index 2f4c935..829e976 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -795,12 +795,21 @@ void
affs_free_prealloc(struct inode *inode)
{
struct super_block *sb = inode->i_sb;
+ u32 first, cnt;
pr_debug("AFFS: free_prealloc(ino=%lu)\n", inode->i_ino);
- while (AFFS_I(inode)->i_pa_cnt) {
- AFFS_I(inode)->i_pa_cnt--;
- affs_free_block(sb, ++AFFS_I(inode)->i_lastalloc);
+ spin_lock(&AFFS_I(inode)->i_alloc);
+ first = AFFS_I(inode)->i_lastalloc;
+ cnt = AFFS_I(inode)->i_pa_cnt;
+ AFFS_I(inode)->i_lastalloc += cnt;
+ AFFS_I(inode)->i_pa_cnt = 0;
+
+ spin_unlock(&AFFS_I(inode)->i_alloc);
+
+ while (cnt) {
+ cnt--;
+ affs_free_block(sb, ++first);
}
}
diff --git a/fs/affs/super.c b/fs/affs/super.c
index 0782653..1df3c95 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -91,6 +91,7 @@ static struct inode *affs_alloc_inode(struct super_block *sb)
i->i_lc = NULL;
i->i_ext_bh = NULL;
i->i_pa_cnt = 0;
+ spin_lock_init(&i->i_alloc);
return &i->vfs_inode;
}
On Sun 13-05-12 15:44:33, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> AFFS code preallocates several blocks as an optimisation. Unfortunately
> it's not protected by lock so the same blocks may end up allocated twice.
> Here is a fix.
>
> Signed-off-by: Vladimir Serbinenko <[email protected]>
The patch looks good to me now. Thanks! You can add:
Reviewed-by: Jan Kara <[email protected]>
Al, will you merge this patch through your tree? AFFS does not seem to
have a maintainer so you are a default fallback...
Honza
> diff --git a/fs/affs/affs.h b/fs/affs/affs.h
> index 45a0ce4..fc1d4ca 100644
> --- a/fs/affs/affs.h
> +++ b/fs/affs/affs.h
> @@ -66,6 +66,8 @@ struct affs_inode_info {
> u32 i_protect; /* unused attribute bits */
> u32 i_lastalloc; /* last allocated block */
> int i_pa_cnt; /* number of preallocated blocks */
> + spinlock_t i_alloc; /* Protects last 2 fields. */
> +
> struct inode vfs_inode;
> };
>
> diff --git a/fs/affs/bitmap.c b/fs/affs/bitmap.c
> index 3e26271..3341bde 100644
> --- a/fs/affs/bitmap.c
> +++ b/fs/affs/bitmap.c
> @@ -151,12 +151,18 @@ affs_alloc_block(struct inode *inode, u32 goal)
>
> pr_debug("AFFS: balloc(inode=%lu,goal=%u): ", inode->i_ino, goal);
>
> + spin_lock(&AFFS_I(inode)->i_alloc);
> +
> if (AFFS_I(inode)->i_pa_cnt) {
> - pr_debug("%d\n", AFFS_I(inode)->i_lastalloc+1);
> + u32 ret;
> AFFS_I(inode)->i_pa_cnt--;
> - return ++AFFS_I(inode)->i_lastalloc;
> + ret = ++AFFS_I(inode)->i_lastalloc;
> + spin_unlock(&AFFS_I(inode)->i_alloc);
> + return ret;
> }
>
> + spin_unlock(&AFFS_I(inode)->i_alloc);
> +
> if (!goal || goal > sbi->s_partition_size) {
> if (goal)
> affs_warning(sb, "affs_balloc", "invalid goal %d", goal);
> @@ -230,16 +236,22 @@ find_bit:
> bit = ffs(tmp & mask) - 1;
> blk += bit + sbi->s_reserved;
> mask2 = mask = 1 << (bit & 31);
> - AFFS_I(inode)->i_lastalloc = blk;
> -
> - /* prealloc as much as possible within this word */
> - while ((mask2 <<= 1)) {
> - if (!(tmp & mask2))
> - break;
> - AFFS_I(inode)->i_pa_cnt++;
> - mask |= mask2;
> +
> + spin_lock(&AFFS_I(inode)->i_alloc);
> + if (!AFFS_I(inode)->i_pa_cnt) {
> + AFFS_I(inode)->i_lastalloc = blk;
> +
> + /* prealloc as much as possible within this word */
> + while ((mask2 <<= 1)) {
> + if (!(tmp & mask2))
> + break;
> + AFFS_I(inode)->i_pa_cnt++;
> + mask |= mask2;
> + }
> + bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
> }
> - bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
> +
> + spin_unlock(&AFFS_I(inode)->i_alloc);
>
> *data = cpu_to_be32(tmp & ~mask);
>
> diff --git a/fs/affs/file.c b/fs/affs/file.c
> index 2f4c935..829e976 100644
> --- a/fs/affs/file.c
> +++ b/fs/affs/file.c
> @@ -795,12 +795,21 @@ void
> affs_free_prealloc(struct inode *inode)
> {
> struct super_block *sb = inode->i_sb;
> + u32 first, cnt;
>
> pr_debug("AFFS: free_prealloc(ino=%lu)\n", inode->i_ino);
>
> - while (AFFS_I(inode)->i_pa_cnt) {
> - AFFS_I(inode)->i_pa_cnt--;
> - affs_free_block(sb, ++AFFS_I(inode)->i_lastalloc);
> + spin_lock(&AFFS_I(inode)->i_alloc);
> + first = AFFS_I(inode)->i_lastalloc;
> + cnt = AFFS_I(inode)->i_pa_cnt;
> + AFFS_I(inode)->i_lastalloc += cnt;
> + AFFS_I(inode)->i_pa_cnt = 0;
> +
> + spin_unlock(&AFFS_I(inode)->i_alloc);
> +
> + while (cnt) {
> + cnt--;
> + affs_free_block(sb, ++first);
> }
> }
>
> diff --git a/fs/affs/super.c b/fs/affs/super.c
> index 0782653..1df3c95 100644
> --- a/fs/affs/super.c
> +++ b/fs/affs/super.c
> @@ -91,6 +91,7 @@ static struct inode *affs_alloc_inode(struct super_block *sb)
> i->i_lc = NULL;
> i->i_ext_bh = NULL;
> i->i_pa_cnt = 0;
> + spin_lock_init(&i->i_alloc);
>
> return &i->vfs_inode;
> }
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
2012/5/14 Jan Kara <[email protected]>:
> On Sun 13-05-12 15:44:33, Vladimir '?-coder/phcoder' Serbinenko wrote:
>> AFFS code preallocates several blocks as an optimisation. Unfortunately
>> it's not protected by lock so the same blocks may end up allocated twice.
>> Here is a fix.
>>
>> Signed-off-by: Vladimir Serbinenko <[email protected]>
> ?The patch looks good to me now. Thanks! You can add:
> Reviewed-by: Jan Kara <[email protected]>
>
> ?Al, will you merge this patch through your tree? AFFS does not seem to
> have a maintainer so you are a default fallback...
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?Honza
>
I don't know the AFFS code, so only a question. Instead to use a spin
lock, I think we can use a simple mutex. Or is the spin lock
mandatory?
Marco
On Mon 14-05-12 12:40:45, Marco Stornelli wrote:
> 2012/5/14 Jan Kara <[email protected]>:
> > On Sun 13-05-12 15:44:33, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> AFFS code preallocates several blocks as an optimisation. Unfortunately
> >> it's not protected by lock so the same blocks may end up allocated twice.
> >> Here is a fix.
> >>
> >> Signed-off-by: Vladimir Serbinenko <[email protected]>
> > The patch looks good to me now. Thanks! You can add:
> > Reviewed-by: Jan Kara <[email protected]>
> >
> > Al, will you merge this patch through your tree? AFFS does not seem to
> > have a maintainer so you are a default fallback...
> >
> > Honza
> >
>
> I don't know the AFFS code, so only a question. Instead to use a spin
> lock, I think we can use a simple mutex. Or is the spin lock
> mandatory?
So what would be an advantage of a mutex? Spinlock *is* the simple locking
variant...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 14.05.2012 12:40, Marco Stornelli wrote:
> 2012/5/14 Jan Kara <[email protected]>:
>> On Sun 13-05-12 15:44:33, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> AFFS code preallocates several blocks as an optimisation. Unfortunately
>>> it's not protected by lock so the same blocks may end up allocated twice.
>>> Here is a fix.
>>>
>>> Signed-off-by: Vladimir Serbinenko <[email protected]>
>> The patch looks good to me now. Thanks! You can add:
>> Reviewed-by: Jan Kara <[email protected]>
>>
>> Al, will you merge this patch through your tree? AFFS does not seem to
>> have a maintainer so you are a default fallback...
>>
>> Honza
>>
>
> I don't know the AFFS code, so only a question. Instead to use a spin
> lock, I think we can use a simple mutex. Or is the spin lock
> mandatory?
My first version used mutex. But then Jan suggested that since the
critical section is very short and doesn't contain any instructions
which might sleep, it's better for performance to use a spin lock.
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
2012/5/14 Jan Kara <[email protected]>:
> On Mon 14-05-12 12:40:45, Marco Stornelli wrote:
>> 2012/5/14 Jan Kara <[email protected]>:
>> > On Sun 13-05-12 15:44:33, Vladimir '?-coder/phcoder' Serbinenko wrote:
>> >> AFFS code preallocates several blocks as an optimisation. Unfortunately
>> >> it's not protected by lock so the same blocks may end up allocated twice.
>> >> Here is a fix.
>> >>
>> >> Signed-off-by: Vladimir Serbinenko <[email protected]>
>> > ?The patch looks good to me now. Thanks! You can add:
>> > Reviewed-by: Jan Kara <[email protected]>
>> >
>> > ?Al, will you merge this patch through your tree? AFFS does not seem to
>> > have a maintainer so you are a default fallback...
>> >
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?Honza
>> >
>>
>> I don't know the AFFS code, so only a question. Instead to use a spin
>> lock, I think we can use a simple mutex. Or is the spin lock
>> mandatory?
> ?So what would be an advantage of a mutex? Spinlock *is* the simple locking
> variant...
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?Honza
> --
None actually, only style, but if there are performance consideration
already done, ok it was only a question. :)
Marco