* [PATCH v2] Fix AFFS race condition.
@ 2012-05-13 13:44 Vladimir 'φ-coder/phcoder' Serbinenko
2012-05-14 9:45 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-13 13:44 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel; +Cc: Al Viro, Josef Bacik, Jan Kara
[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]
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 <phcoder@gmail.com>
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;
}
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] Fix AFFS race condition.
2012-05-13 13:44 [PATCH v2] Fix AFFS race condition Vladimir 'φ-coder/phcoder' Serbinenko
@ 2012-05-14 9:45 ` Jan Kara
2012-05-14 10:40 ` Marco Stornelli
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2012-05-14 9:45 UTC (permalink / raw)
To: Vladimir 'φ-coder/phcoder' Serbinenko
Cc: linux-fsdevel, linux-kernel, Al Viro, Josef Bacik, Jan Kara
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 <phcoder@gmail.com>
The patch looks good to me now. Thanks! You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
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 <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] Fix AFFS race condition.
2012-05-14 9:45 ` Jan Kara
@ 2012-05-14 10:40 ` Marco Stornelli
2012-05-14 10:53 ` Jan Kara
2012-05-14 10:54 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 2 replies; 6+ messages in thread
From: Marco Stornelli @ 2012-05-14 10:40 UTC (permalink / raw)
To: Jan Kara
Cc: Vladimir 'φ-coder/phcoder' Serbinenko, linux-fsdevel,
linux-kernel, Al Viro, Josef Bacik
2012/5/14 Jan Kara <jack@suse.cz>:
> 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 <phcoder@gmail.com>
> The patch looks good to me now. Thanks! You can add:
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Fix AFFS race condition.
2012-05-14 10:40 ` Marco Stornelli
@ 2012-05-14 10:53 ` Jan Kara
2012-05-14 11:06 ` Marco Stornelli
2012-05-14 10:54 ` Vladimir 'φ-coder/phcoder' Serbinenko
1 sibling, 1 reply; 6+ messages in thread
From: Jan Kara @ 2012-05-14 10:53 UTC (permalink / raw)
To: Marco Stornelli
Cc: Jan Kara, Vladimir 'φ-coder/phcoder' Serbinenko,
linux-fsdevel, linux-kernel, Al Viro, Josef Bacik
On Mon 14-05-12 12:40:45, Marco Stornelli wrote:
> 2012/5/14 Jan Kara <jack@suse.cz>:
> > 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 <phcoder@gmail.com>
> > The patch looks good to me now. Thanks! You can add:
> > Reviewed-by: Jan Kara <jack@suse.cz>
> >
> > 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 <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Fix AFFS race condition.
2012-05-14 10:53 ` Jan Kara
@ 2012-05-14 11:06 ` Marco Stornelli
0 siblings, 0 replies; 6+ messages in thread
From: Marco Stornelli @ 2012-05-14 11:06 UTC (permalink / raw)
To: Jan Kara
Cc: Vladimir 'φ-coder/phcoder' Serbinenko, linux-fsdevel,
linux-kernel, Al Viro, Josef Bacik
2012/5/14 Jan Kara <jack@suse.cz>:
> On Mon 14-05-12 12:40:45, Marco Stornelli wrote:
>> 2012/5/14 Jan Kara <jack@suse.cz>:
>> > 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 <phcoder@gmail.com>
>> > The patch looks good to me now. Thanks! You can add:
>> > Reviewed-by: Jan Kara <jack@suse.cz>
>> >
>> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Fix AFFS race condition.
2012-05-14 10:40 ` Marco Stornelli
2012-05-14 10:53 ` Jan Kara
@ 2012-05-14 10:54 ` Vladimir 'φ-coder/phcoder' Serbinenko
1 sibling, 0 replies; 6+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-14 10:54 UTC (permalink / raw)
To: Marco Stornelli
Cc: Jan Kara, linux-fsdevel, linux-kernel, Al Viro, Josef Bacik
[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]
On 14.05.2012 12:40, Marco Stornelli wrote:
> 2012/5/14 Jan Kara <jack@suse.cz>:
>> 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 <phcoder@gmail.com>
>> The patch looks good to me now. Thanks! You can add:
>> Reviewed-by: Jan Kara <jack@suse.cz>
>>
>> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-14 11:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-13 13:44 [PATCH v2] Fix AFFS race condition Vladimir 'φ-coder/phcoder' Serbinenko
2012-05-14 9:45 ` Jan Kara
2012-05-14 10:40 ` Marco Stornelli
2012-05-14 10:53 ` Jan Kara
2012-05-14 11:06 ` Marco Stornelli
2012-05-14 10:54 ` Vladimir 'φ-coder/phcoder' Serbinenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).