* [PATCH] reiserfs: Fix locking in reiserfs_quota_on() @ 2010-10-28 0:28 Jan Kara 2010-10-28 0:47 ` Al Viro 2010-10-28 1:36 ` Frederic Weisbecker 0 siblings, 2 replies; 5+ messages in thread From: Jan Kara @ 2010-10-28 0:28 UTC (permalink / raw) To: reiserfs-devel; +Cc: linux-fsdevel, Frederic Weisbecker, Jan Kara reiserfs_quota_on() unpacks a tail of quota file in case it has one. But after BKL conversion, reiserfs_unpack() expects to be called with write_lock held. So acquire the lock before calling reiserfs_unpack() to avoid assertion failures. Reported-by: markus.gapp@gmx.net Signed-off-by: Jan Kara <jack@suse.cz> --- fs/reiserfs/super.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) Frederic, would you merge this patch or should I merge it? diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c index 6e85cfd..73c000f 100644 --- a/fs/reiserfs/super.c +++ b/fs/reiserfs/super.c @@ -2059,7 +2059,9 @@ static int reiserfs_quota_on(struct super_block *sb, int type, int format_id, inode = path->dentry->d_inode; /* We must not pack tails for quota files on reiserfs for quota IO to work */ if (!(REISERFS_I(inode)->i_flags & i_nopack_mask)) { + reiserfs_write_lock(sb); err = reiserfs_unpack(inode, NULL); + reiserfs_write_unlock(sb); if (err) { reiserfs_warning(sb, "super-6520", "Unpacking tail of quota file failed" -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] reiserfs: Fix locking in reiserfs_quota_on() 2010-10-28 0:28 [PATCH] reiserfs: Fix locking in reiserfs_quota_on() Jan Kara @ 2010-10-28 0:47 ` Al Viro 2010-10-28 1:36 ` Frederic Weisbecker 1 sibling, 0 replies; 5+ messages in thread From: Al Viro @ 2010-10-28 0:47 UTC (permalink / raw) To: Jan Kara; +Cc: reiserfs-devel, linux-fsdevel, Frederic Weisbecker BTW, why does reiserfs flips MS_ACTIVE on for a while during mount around the quota work? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] reiserfs: Fix locking in reiserfs_quota_on() 2010-10-28 0:28 [PATCH] reiserfs: Fix locking in reiserfs_quota_on() Jan Kara 2010-10-28 0:47 ` Al Viro @ 2010-10-28 1:36 ` Frederic Weisbecker 2010-11-09 15:37 ` Jan Kara 1 sibling, 1 reply; 5+ messages in thread From: Frederic Weisbecker @ 2010-10-28 1:36 UTC (permalink / raw) To: Jan Kara; +Cc: reiserfs-devel, linux-fsdevel, Al Viro, Andrew Morton On Thu, Oct 28, 2010 at 02:28:23AM +0200, Jan Kara wrote: > reiserfs_quota_on() unpacks a tail of quota file in case it has one. But after > BKL conversion, reiserfs_unpack() expects to be called with write_lock held. > So acquire the lock before calling reiserfs_unpack() to avoid assertion > failures. > > Reported-by: markus.gapp@gmx.net > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/reiserfs/super.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > Frederic, would you merge this patch or should I merge it? > > diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c > index 6e85cfd..73c000f 100644 > --- a/fs/reiserfs/super.c > +++ b/fs/reiserfs/super.c > @@ -2059,7 +2059,9 @@ static int reiserfs_quota_on(struct super_block *sb, int type, int format_id, > inode = path->dentry->d_inode; > /* We must not pack tails for quota files on reiserfs for quota IO to work */ > if (!(REISERFS_I(inode)->i_flags & i_nopack_mask)) { > + reiserfs_write_lock(sb); > err = reiserfs_unpack(inode, NULL); > + reiserfs_write_unlock(sb); > if (err) { > reiserfs_warning(sb, "super-6520", > "Unpacking tail of quota file failed" > -- > 1.7.1 > Yeah. This is due to a recent fix in reiserfs_unpack(). And in this fix I assumed reiserfs_unpack() was always called under the reiserfs lock. I was wrong, I thought that reiserfs_quota_on() was ok because it can call joural_begin() which appears to have the same requirements. But no that's probably another bug, journal_begin() should also be called under the reiserfs lock. Anyway that must be another patch. For this specific problem, it might be slightly more proper to do the below. It lowers a bit the reiserfs lock coverage and also fixes a weird lock-unlock ordering in reiserfs_unpack() that was doing: Lock A - Lock B - Unlock A - Unlock B Hmm? diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c index 5cbb81e..af2a58f 100644 --- a/fs/reiserfs/ioctl.c +++ b/fs/reiserfs/ioctl.c @@ -189,8 +189,8 @@ int reiserfs_unpack(struct inode *inode, struct file *filp) /* we need to make sure nobody is changing the file size beneath ** us */ - reiserfs_mutex_lock_safe(&inode->i_mutex, inode->i_sb); depth = reiserfs_write_lock_once(inode->i_sb); + reiserfs_mutex_lock_safe(&inode->i_mutex, inode->i_sb); write_from = inode->i_size & (blocksize - 1); /* if we are on a block boundary, we are already unpacked. */ ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] reiserfs: Fix locking in reiserfs_quota_on() 2010-10-28 1:36 ` Frederic Weisbecker @ 2010-11-09 15:37 ` Jan Kara 2010-11-09 15:40 ` Frederic Weisbecker 0 siblings, 1 reply; 5+ messages in thread From: Jan Kara @ 2010-11-09 15:37 UTC (permalink / raw) To: Frederic Weisbecker Cc: Jan Kara, reiserfs-devel, linux-fsdevel, Al Viro, Andrew Morton On Thu 28-10-10 03:36:07, Frederic Weisbecker wrote: > On Thu, Oct 28, 2010 at 02:28:23AM +0200, Jan Kara wrote: > > reiserfs_quota_on() unpacks a tail of quota file in case it has one. But after > > BKL conversion, reiserfs_unpack() expects to be called with write_lock held. > > So acquire the lock before calling reiserfs_unpack() to avoid assertion > > failures. > > > > Reported-by: markus.gapp@gmx.net > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/reiserfs/super.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > Frederic, would you merge this patch or should I merge it? > > > > diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c > > index 6e85cfd..73c000f 100644 > > --- a/fs/reiserfs/super.c > > +++ b/fs/reiserfs/super.c > > @@ -2059,7 +2059,9 @@ static int reiserfs_quota_on(struct super_block *sb, int type, int format_id, > > inode = path->dentry->d_inode; > > /* We must not pack tails for quota files on reiserfs for quota IO to work */ > > if (!(REISERFS_I(inode)->i_flags & i_nopack_mask)) { > > + reiserfs_write_lock(sb); > > err = reiserfs_unpack(inode, NULL); > > + reiserfs_write_unlock(sb); > > if (err) { > > reiserfs_warning(sb, "super-6520", > > "Unpacking tail of quota file failed" > > -- > > 1.7.1 > > > > > > Yeah. This is due to a recent fix in reiserfs_unpack(). > And in this fix I assumed reiserfs_unpack() was always called under the > reiserfs lock. > I was wrong, I thought that reiserfs_quota_on() was ok because it can > call joural_begin() which appears to have the same requirements. > But no that's probably another bug, journal_begin() should also > be called under the reiserfs lock. > Anyway that must be another patch. > > > For this specific problem, it might be slightly more proper to do the > below. It lowers a bit the reiserfs lock coverage and also fixes > a weird lock-unlock ordering in reiserfs_unpack() that was doing: > > Lock A - Lock B - Unlock A - Unlock B > > Hmm? Looks OK to me. Do you plan to merge it? Honza > diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c > index 5cbb81e..af2a58f 100644 > --- a/fs/reiserfs/ioctl.c > +++ b/fs/reiserfs/ioctl.c > @@ -189,8 +189,8 @@ int reiserfs_unpack(struct inode *inode, struct file *filp) > /* we need to make sure nobody is changing the file size beneath > ** us > */ > - reiserfs_mutex_lock_safe(&inode->i_mutex, inode->i_sb); > depth = reiserfs_write_lock_once(inode->i_sb); > + reiserfs_mutex_lock_safe(&inode->i_mutex, inode->i_sb); > > write_from = inode->i_size & (blocksize - 1); > /* if we are on a block boundary, we are already unpacked. */ > > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] reiserfs: Fix locking in reiserfs_quota_on() 2010-11-09 15:37 ` Jan Kara @ 2010-11-09 15:40 ` Frederic Weisbecker 0 siblings, 0 replies; 5+ messages in thread From: Frederic Weisbecker @ 2010-11-09 15:40 UTC (permalink / raw) To: Jan Kara; +Cc: reiserfs-devel, linux-fsdevel, Al Viro, Andrew Morton On Tue, Nov 09, 2010 at 04:37:40PM +0100, Jan Kara wrote: > On Thu 28-10-10 03:36:07, Frederic Weisbecker wrote: > > On Thu, Oct 28, 2010 at 02:28:23AM +0200, Jan Kara wrote: > > > reiserfs_quota_on() unpacks a tail of quota file in case it has one. But after > > > BKL conversion, reiserfs_unpack() expects to be called with write_lock held. > > > So acquire the lock before calling reiserfs_unpack() to avoid assertion > > > failures. > > > > > > Reported-by: markus.gapp@gmx.net > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > fs/reiserfs/super.c | 2 ++ > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > > > Frederic, would you merge this patch or should I merge it? > > > > > > diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c > > > index 6e85cfd..73c000f 100644 > > > --- a/fs/reiserfs/super.c > > > +++ b/fs/reiserfs/super.c > > > @@ -2059,7 +2059,9 @@ static int reiserfs_quota_on(struct super_block *sb, int type, int format_id, > > > inode = path->dentry->d_inode; > > > /* We must not pack tails for quota files on reiserfs for quota IO to work */ > > > if (!(REISERFS_I(inode)->i_flags & i_nopack_mask)) { > > > + reiserfs_write_lock(sb); > > > err = reiserfs_unpack(inode, NULL); > > > + reiserfs_write_unlock(sb); > > > if (err) { > > > reiserfs_warning(sb, "super-6520", > > > "Unpacking tail of quota file failed" > > > -- > > > 1.7.1 > > > > > > > > > > > Yeah. This is due to a recent fix in reiserfs_unpack(). > > And in this fix I assumed reiserfs_unpack() was always called under the > > reiserfs lock. > > I was wrong, I thought that reiserfs_quota_on() was ok because it can > > call joural_begin() which appears to have the same requirements. > > But no that's probably another bug, journal_begin() should also > > be called under the reiserfs lock. > > Anyway that must be another patch. > > > > > > For this specific problem, it might be slightly more proper to do the > > below. It lowers a bit the reiserfs lock coverage and also fixes > > a weird lock-unlock ordering in reiserfs_unpack() that was doing: > > > > Lock A - Lock B - Unlock A - Unlock B > > > > Hmm? > Looks OK to me. Do you plan to merge it? Yep, will be sent to Andrew soon. Thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-11-09 15:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-10-28 0:28 [PATCH] reiserfs: Fix locking in reiserfs_quota_on() Jan Kara 2010-10-28 0:47 ` Al Viro 2010-10-28 1:36 ` Frederic Weisbecker 2010-11-09 15:37 ` Jan Kara 2010-11-09 15:40 ` Frederic Weisbecker
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).