Message ID | 20201226095641.17290-1-luoshijie1@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RFC,RESEND] fs: fix a hungtask problem when freeze/unfreeze fs | expand |
On Sat, Dec 26, 2020 at 04:56:41AM -0500, Shijie Luo wrote: > The root cause is that when offline/onlines disks, the filesystem can easily get into > a error state and this makes it change to be read-only. Function freeze_super() will hold > all sb_writers rwsems including rwsem of SB_FREEZE_WRITE when filesystem not read-only, > but thaw_super_locked() cannot release these while the filesystem suddenly become read-only, > because the logic will go to out. > > freeze_super > hold sb_writers rwsems > sb->s_writers.frozen = SB_FREEZE_COMPLETE > thaw_super_locked > sb_rdonly > sb->s_writers.frozen = SB_UNFROZEN; > goto out // not release rwsems > > And at this time, if we call mnt_want_write(), the process will be blocked. > > This patch fixes this problem, when filesystem is read-only, just not to set sb_writers.frozen > be SB_FREEZE_COMPLETE in freeze_super() and then release all rwsems in thaw_super_locked. I really don't like that - you end up with a case when freeze_super() returns 0 *and* consumes the reference it had been give. > if (sb_rdonly(sb)) { > - /* Nothing to do really... */ > - sb->s_writers.frozen = SB_FREEZE_COMPLETE; > - up_write(&sb->s_umount); > + deactivate_locked_super(sb); > return 0; > }
On 2020/12/26 23:55, Al Viro wrote: > On Sat, Dec 26, 2020 at 04:56:41AM -0500, Shijie Luo wrote: > >> The root cause is that when offline/onlines disks, the filesystem can easily get into >> a error state and this makes it change to be read-only. Function freeze_super() will hold >> all sb_writers rwsems including rwsem of SB_FREEZE_WRITE when filesystem not read-only, >> but thaw_super_locked() cannot release these while the filesystem suddenly become read-only, >> because the logic will go to out. >> >> freeze_super >> hold sb_writers rwsems >> sb->s_writers.frozen = SB_FREEZE_COMPLETE >> thaw_super_locked >> sb_rdonly >> sb->s_writers.frozen = SB_UNFROZEN; >> goto out // not release rwsems >> >> And at this time, if we call mnt_want_write(), the process will be blocked. >> >> This patch fixes this problem, when filesystem is read-only, just not to set sb_writers.frozen >> be SB_FREEZE_COMPLETE in freeze_super() and then release all rwsems in thaw_super_locked. > I really don't like that - you end up with a case when freeze_super() returns 0 *and* > consumes the reference it had been give. Consuming the reference here because we won't "set frozen = SB_FREEZE_COMPLETE" in thaw_super_locked() now. If don't do that, we never have a chance to consume it, thaw_super_locked() will directly return "-EINVAL". But I do agree that it's not a good idea to return 0. How about returning "-EINVAL or -ENOTSUPP" ? And, If we keep "frozen = SB_FREEZE_COMPLETE" in freeze_super() here, it will cause another problem, thaw_super_locked() will always release rwsems in my logic, but freeze_super() won't acquire the rwsems when filesystem is read-only. Thanks. >> if (sb_rdonly(sb)) { >> - /* Nothing to do really... */ >> - sb->s_writers.frozen = SB_FREEZE_COMPLETE; >> - up_write(&sb->s_umount); >> + deactivate_locked_super(sb); >> return 0; >> } > . >
On Mon 28-12-20 10:15:16, Shijie Luo wrote: > > On 2020/12/26 23:55, Al Viro wrote: > > On Sat, Dec 26, 2020 at 04:56:41AM -0500, Shijie Luo wrote: > > > > > The root cause is that when offline/onlines disks, the filesystem can easily get into > > > a error state and this makes it change to be read-only. Function freeze_super() will hold > > > all sb_writers rwsems including rwsem of SB_FREEZE_WRITE when filesystem not read-only, > > > but thaw_super_locked() cannot release these while the filesystem suddenly become read-only, > > > because the logic will go to out. > > > > > > freeze_super > > > hold sb_writers rwsems > > > sb->s_writers.frozen = SB_FREEZE_COMPLETE > > > thaw_super_locked > > > sb_rdonly > > > sb->s_writers.frozen = SB_UNFROZEN; > > > goto out // not release rwsems > > > > > > And at this time, if we call mnt_want_write(), the process will be blocked. > > > > > > This patch fixes this problem, when filesystem is read-only, just not to set sb_writers.frozen > > > be SB_FREEZE_COMPLETE in freeze_super() and then release all rwsems in thaw_super_locked. > > I really don't like that - you end up with a case when freeze_super() returns 0 *and* > > consumes the reference it had been give. > > Consuming the reference here because we won't "set frozen = > SB_FREEZE_COMPLETE" in thaw_super_locked() now. > > If don't do that, we never have a chance to consume it, thaw_super_locked() > will directly return "-EINVAL". But I do > > agree that it's not a good idea to return 0. How about returning "-EINVAL or > -ENOTSUPP" ? > > And, If we keep "frozen = SB_FREEZE_COMPLETE" in freeze_super() here, it > will cause another problem, thaw_super_locked() > > will always release rwsems in my logic, but freeze_super() won't acquire the > rwsems when filesystem is read-only. I was thinking about this for a while. I think the best solution would be to track whether the fs was read only (and thus frozen without locking percpu semaphores) at the time of freezing. I'm attaching that patch. Does it fix your problem? Honza
Hi! On 2021/1/5 0:04, Jan Kara wrote: >> Consuming the reference here because we won't "set frozen = >> SB_FREEZE_COMPLETE" in thaw_super_locked() now. >> >> If don't do that, we never have a chance to consume it, thaw_super_locked() >> will directly return "-EINVAL". But I do >> >> agree that it's not a good idea to return 0. How about returning "-EINVAL or >> -ENOTSUPP" ? >> >> And, If we keep "frozen = SB_FREEZE_COMPLETE" in freeze_super() here, it >> will cause another problem, thaw_super_locked() >> >> will always release rwsems in my logic, but freeze_super() won't acquire the >> rwsems when filesystem is read-only. > I was thinking about this for a while. I think the best solution would be > to track whether the fs was read only (and thus frozen without locking > percpu semaphores) at the time of freezing. I'm attaching that patch. Does > it fix your problem? > > Honza I tested your patch, and it can indeed fix this deadlock problem. Thanks.
On Tue 05-01-21 10:48:48, Shijie Luo wrote: > Hi! > > On 2021/1/5 0:04, Jan Kara wrote: > > > Consuming the reference here because we won't "set frozen = > > > SB_FREEZE_COMPLETE" in thaw_super_locked() now. > > > > > > If don't do that, we never have a chance to consume it, thaw_super_locked() > > > will directly return "-EINVAL". But I do > > > > > > agree that it's not a good idea to return 0. How about returning "-EINVAL or > > > -ENOTSUPP" ? > > > > > > And, If we keep "frozen = SB_FREEZE_COMPLETE" in freeze_super() here, it > > > will cause another problem, thaw_super_locked() > > > > > > will always release rwsems in my logic, but freeze_super() won't acquire the > > > rwsems when filesystem is read-only. > > I was thinking about this for a while. I think the best solution would be > > to track whether the fs was read only (and thus frozen without locking > > percpu semaphores) at the time of freezing. I'm attaching that patch. Does > > it fix your problem? > > > > Honza > > I tested your patch, and it can indeed fix this deadlock problem. Thanks for testing! I've sent the patch to Al for inclusion. Honza
diff --git a/fs/super.c b/fs/super.c index 2c6cdea2ab2d..50d79213f678 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1672,9 +1672,7 @@ int freeze_super(struct super_block *sb) } if (sb_rdonly(sb)) { - /* Nothing to do really... */ - sb->s_writers.frozen = SB_FREEZE_COMPLETE; - up_write(&sb->s_umount); + deactivate_locked_super(sb); return 0; } @@ -1733,13 +1731,11 @@ static int thaw_super_locked(struct super_block *sb) return -EINVAL; } - if (sb_rdonly(sb)) { - sb->s_writers.frozen = SB_UNFROZEN; - goto out; - } - lockdep_sb_freeze_acquire(sb); + if (sb_rdonly(sb)) + goto out; + if (sb->s_op->unfreeze_fs) { error = sb->s_op->unfreeze_fs(sb); if (error) { @@ -1751,9 +1747,9 @@ static int thaw_super_locked(struct super_block *sb) } } +out: sb->s_writers.frozen = SB_UNFROZEN; sb_freeze_unlock(sb); -out: wake_up(&sb->s_writers.wait_unfrozen); deactivate_locked_super(sb); return 0;