From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH v2] Fix AFFS race condition. Date: Mon, 14 May 2012 11:45:43 +0200 Message-ID: <20120514094543.GC5353@quack.suse.cz> References: <4FAFBAC1.20603@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Al Viro , Josef Bacik , Jan Kara To: Vladimir =?utf-8?Q?'=CF=86-coder=2Fphcoder'?= Serbinenko Return-path: Content-Disposition: inline In-Reply-To: <4FAFBAC1.20603@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Sun 13-05-12 15:44:33, Vladimir '=CF=86-coder/phcoder' Serbinenko wr= ote: > AFFS code preallocates several blocks as an optimisation. Unfortunate= ly > it's not protected by lock so the same blocks may end up allocated tw= ice. > Here is a fix. >=20 > Signed-off-by: Vladimir Serbinenko The patch looks good to me now. Thanks! You can add: Reviewed-by: Jan Kara Al, will you merge this patch through your tree? AFFS does not seem t= o 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; > }; > =20 > 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) > =20 > pr_debug("AFFS: balloc(inode=3D%lu,goal=3D%u): ", inode->i_ino, goa= l); > =20 > + 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 =3D ++AFFS_I(inode)->i_lastalloc; > + spin_unlock(&AFFS_I(inode)->i_alloc); > + return ret; > } > =20 > + 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 =3D ffs(tmp & mask) - 1; > blk +=3D bit + sbi->s_reserved; > mask2 =3D mask =3D 1 << (bit & 31); > - AFFS_I(inode)->i_lastalloc =3D blk; > - > - /* prealloc as much as possible within this word */ > - while ((mask2 <<=3D 1)) { > - if (!(tmp & mask2)) > - break; > - AFFS_I(inode)->i_pa_cnt++; > - mask |=3D mask2; > + > + spin_lock(&AFFS_I(inode)->i_alloc); > + if (!AFFS_I(inode)->i_pa_cnt) { > + AFFS_I(inode)->i_lastalloc =3D blk; > + > + /* prealloc as much as possible within this word */ > + while ((mask2 <<=3D 1)) { > + if (!(tmp & mask2)) > + break; > + AFFS_I(inode)->i_pa_cnt++; > + mask |=3D mask2; > + } > + bm->bm_free -=3D AFFS_I(inode)->i_pa_cnt + 1; > } > - bm->bm_free -=3D AFFS_I(inode)->i_pa_cnt + 1; > + > + spin_unlock(&AFFS_I(inode)->i_alloc); > =20 > *data =3D cpu_to_be32(tmp & ~mask); > =20 > 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 =3D inode->i_sb; > + u32 first, cnt; > =20 > pr_debug("AFFS: free_prealloc(ino=3D%lu)\n", inode->i_ino); > =20 > - 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 =3D AFFS_I(inode)->i_lastalloc; > + cnt =3D AFFS_I(inode)->i_pa_cnt; > + AFFS_I(inode)->i_lastalloc +=3D cnt; > + AFFS_I(inode)->i_pa_cnt =3D 0; > + > + spin_unlock(&AFFS_I(inode)->i_alloc); > + > + while (cnt) { > + cnt--; > + affs_free_block(sb, ++first); > } > } > =20 > 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 =3D NULL; > i->i_ext_bh =3D NULL; > i->i_pa_cnt =3D 0; > + spin_lock_init(&i->i_alloc); > =20 > return &i->vfs_inode; > } >=20 --=20 Jan Kara SUSE Labs, CR