| Message ID | 20150214185524.GA16579@p183.telecom.by (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat 14-02-15 21:55:24, Alexey Dobriyan wrote: > Freezing and thawing are separate system calls, task which is supposed > to thaw filesystem/superblock can disappear due to crash or not thaw > due to a bug. Record at least task name (we can't take task_struct > reference) to make support engineer's life easier. > > Hopefully 16 bytes per superblock isn't much. > > P.S.: Cc'ing GFS2 people just in case they want to correct > my understanding of GFS2 having async freeze code. > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> Hum, and when do you show the task name? Or do you expect that customer takes a crashdump and support just finds it in memory? > --- > > fs/ioctl.c | 22 ++++++++++++++++------ > fs/super.c | 2 ++ > include/linux/fs.h | 2 ++ > 3 files changed, 20 insertions(+), 6 deletions(-) > > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > static int ioctl_fsfreeze(struct file *filp) > { > struct super_block *sb = file_inode(filp)->i_sb; > + int rv; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) > return -EOPNOTSUPP; > > /* Freeze */ > - if (sb->s_op->freeze_super) > - return sb->s_op->freeze_super(sb); > - return freeze_super(sb); > + if (sb->s_op->freeze_super) { > + rv = sb->s_op->freeze_super(sb); > + if (rv == 0) > + get_task_comm(sb->s_writers.freeze_comm, current); > + } else > + rv = freeze_super(sb); > + return rv; Why don't you just set the name in ioctl_fsfreeze() in both cases? Also you seem to be missing freezing / thawing in freeze/thaw_bdev() functions. > } > > static int ioctl_fsthaw(struct file *filp) > { > struct super_block *sb = file_inode(filp)->i_sb; > + int rv; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > /* Thaw */ > - if (sb->s_op->thaw_super) > - return sb->s_op->thaw_super(sb); > - return thaw_super(sb); > + if (sb->s_op->thaw_super) { > + rv = sb->s_op->thaw_super(sb); > + if (rv == 0) > + memset(sb->s_writers.freeze_comm, 0, TASK_COMM_LEN); > + } else > + rv = thaw_super(sb); > + return rv; > } > > /* > --- a/fs/super.c > +++ b/fs/super.c > @@ -1355,6 +1355,7 @@ int freeze_super(struct super_block *sb) > * sees write activity when frozen is set to SB_FREEZE_COMPLETE. > */ > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > + get_task_comm(sb->s_writers.freeze_comm, current); > up_write(&sb->s_umount); > return 0; > } > @@ -1391,6 +1392,7 @@ int thaw_super(struct super_block *sb) > > out: > sb->s_writers.frozen = SB_UNFROZEN; > + memset(sb->s_writers.freeze_comm, 0, TASK_COMM_LEN); > smp_wmb(); > wake_up(&sb->s_writers.wait_unfrozen); > deactivate_locked_super(sb); > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1221,6 +1221,8 @@ struct sb_writers { > int frozen; /* Is sb frozen? */ > wait_queue_head_t wait_unfrozen; /* queue for waiting for > sb to be thawed */ > + /* who froze superblock */ > + char freeze_comm[16]; Here should be TASK_COMM_LEN, shouldn't it? Honza
On Mon, Feb 16, 2015 at 10:38:52AM +0100, Jan Kara wrote: > On Sat 14-02-15 21:55:24, Alexey Dobriyan wrote: > > Freezing and thawing are separate system calls, task which is supposed > > to thaw filesystem/superblock can disappear due to crash or not thaw > > due to a bug. Record at least task name (we can't take task_struct > > reference) to make support engineer's life easier. > > > > Hopefully 16 bytes per superblock isn't much. > > > > P.S.: Cc'ing GFS2 people just in case they want to correct > > my understanding of GFS2 having async freeze code. > > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > Hum, and when do you show the task name? Or do you expect that customer > takes a crashdump and support just finds it in memory? Yeah, having at least something in crashdump is fine. > > --- a/fs/ioctl.c > > +++ b/fs/ioctl.c > > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > > static int ioctl_fsfreeze(struct file *filp) > > { > > struct super_block *sb = file_inode(filp)->i_sb; > > + int rv; > > > > if (!capable(CAP_SYS_ADMIN)) > > return -EPERM; > > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) > > return -EOPNOTSUPP; > > > > /* Freeze */ > > - if (sb->s_op->freeze_super) > > - return sb->s_op->freeze_super(sb); > > - return freeze_super(sb); > > + if (sb->s_op->freeze_super) { > > + rv = sb->s_op->freeze_super(sb); > > + if (rv == 0) > > + get_task_comm(sb->s_writers.freeze_comm, current); > > + } else > > + rv = freeze_super(sb); > > + return rv; > Why don't you just set the name in ioctl_fsfreeze() in both cases? There are users of freeze_super() in GFS2 unless I'm misreading code. > Also you seem to be missing freezing / thawing in freeze/thaw_bdev() > functions. You are correct. Resending patch (blockdev freezing tested with XFS). > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1221,6 +1221,8 @@ struct sb_writers { > > int frozen; /* Is sb frozen? */ > > wait_queue_head_t wait_unfrozen; /* queue for waiting for > > sb to be thawed */ > > + /* who froze superblock */ > > + char freeze_comm[16]; > Here should be TASK_COMM_LEN, shouldn't it? It will pull sched.h, dunno if we care about headers anymore. Alexey -- 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
On Wed 18-02-15 10:34:55, Alexey Dobriyan wrote: > On Mon, Feb 16, 2015 at 10:38:52AM +0100, Jan Kara wrote: > > On Sat 14-02-15 21:55:24, Alexey Dobriyan wrote: > > > Freezing and thawing are separate system calls, task which is supposed > > > to thaw filesystem/superblock can disappear due to crash or not thaw > > > due to a bug. Record at least task name (we can't take task_struct > > > reference) to make support engineer's life easier. > > > > > > Hopefully 16 bytes per superblock isn't much. > > > > > > P.S.: Cc'ing GFS2 people just in case they want to correct > > > my understanding of GFS2 having async freeze code. > > > > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > Hum, and when do you show the task name? Or do you expect that customer > > takes a crashdump and support just finds it in memory? > > Yeah, having at least something in crashdump is fine. OK, then comment about this at freeze_comm[] definition so that it's clear it isn't just set-but-never-read field. > > > --- a/fs/ioctl.c > > > +++ b/fs/ioctl.c > > > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > > > static int ioctl_fsfreeze(struct file *filp) > > > { > > > struct super_block *sb = file_inode(filp)->i_sb; > > > + int rv; > > > > > > if (!capable(CAP_SYS_ADMIN)) > > > return -EPERM; > > > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) > > > return -EOPNOTSUPP; > > > > > > /* Freeze */ > > > - if (sb->s_op->freeze_super) > > > - return sb->s_op->freeze_super(sb); > > > - return freeze_super(sb); > > > + if (sb->s_op->freeze_super) { > > > + rv = sb->s_op->freeze_super(sb); > > > + if (rv == 0) > > > + get_task_comm(sb->s_writers.freeze_comm, current); > > > + } else > > > + rv = freeze_super(sb); > > > + return rv; > > Why don't you just set the name in ioctl_fsfreeze() in both cases? > > There are users of freeze_super() in GFS2 unless I'm misreading code. Yes, there are. The call in fs/gfs2/glops.c is in a call path from ->freeze_super() handler for GFS2 so that one is handled in ioctl_fsfreeze() anyway. The call in fs/gfs2/sys.c is a way to freeze filesystem via sysfs (dunno why GFS2 has to invent its own thing and ioctl isn't enough). Steven? So having the logic in ioctl_fsfreeze(), freeze_bdev() and freeze_store() in gfs2 seems to be enough. > > Also you seem to be missing freezing / thawing in freeze/thaw_bdev() > > functions. > > You are correct. Resending patch (blockdev freezing tested with XFS). > > > > --- a/include/linux/fs.h > > > +++ b/include/linux/fs.h > > > @@ -1221,6 +1221,8 @@ struct sb_writers { > > > int frozen; /* Is sb frozen? */ > > > wait_queue_head_t wait_unfrozen; /* queue for waiting for > > > sb to be thawed */ > > > + /* who froze superblock */ > > > + char freeze_comm[16]; > > Here should be TASK_COMM_LEN, shouldn't it? > > It will pull sched.h, dunno if we care about headers anymore. That's not ideal but IMHO better than having the value hardcoded here. That is pretty fragile - i.e. think what happens when someone decides to increase TASK_COMM_LEN... Honza
Hi, On 18/02/15 09:13, Jan Kara wrote: > On Wed 18-02-15 10:34:55, Alexey Dobriyan wrote: >> On Mon, Feb 16, 2015 at 10:38:52AM +0100, Jan Kara wrote: >>> On Sat 14-02-15 21:55:24, Alexey Dobriyan wrote: >>>> Freezing and thawing are separate system calls, task which is supposed >>>> to thaw filesystem/superblock can disappear due to crash or not thaw >>>> due to a bug. Record at least task name (we can't take task_struct >>>> reference) to make support engineer's life easier. >>>> >>>> Hopefully 16 bytes per superblock isn't much. >>>> >>>> P.S.: Cc'ing GFS2 people just in case they want to correct >>>> my understanding of GFS2 having async freeze code. >>>> >>>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> >>> Hum, and when do you show the task name? Or do you expect that customer >>> takes a crashdump and support just finds it in memory? >> Yeah, having at least something in crashdump is fine. > OK, then comment about this at freeze_comm[] definition so that it's > clear it isn't just set-but-never-read field. > >>>> --- a/fs/ioctl.c >>>> +++ b/fs/ioctl.c >>>> @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, >>>> static int ioctl_fsfreeze(struct file *filp) >>>> { >>>> struct super_block *sb = file_inode(filp)->i_sb; >>>> + int rv; >>>> >>>> if (!capable(CAP_SYS_ADMIN)) >>>> return -EPERM; >>>> @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) >>>> return -EOPNOTSUPP; >>>> >>>> /* Freeze */ >>>> - if (sb->s_op->freeze_super) >>>> - return sb->s_op->freeze_super(sb); >>>> - return freeze_super(sb); >>>> + if (sb->s_op->freeze_super) { >>>> + rv = sb->s_op->freeze_super(sb); >>>> + if (rv == 0) >>>> + get_task_comm(sb->s_writers.freeze_comm, current); >>>> + } else >>>> + rv = freeze_super(sb); >>>> + return rv; >>> Why don't you just set the name in ioctl_fsfreeze() in both cases? >> There are users of freeze_super() in GFS2 unless I'm misreading code. > Yes, there are. The call in fs/gfs2/glops.c is in a call path from > ->freeze_super() handler for GFS2 so that one is handled in > ioctl_fsfreeze() anyway. The call in fs/gfs2/sys.c is a way to freeze > filesystem via sysfs (dunno why GFS2 has to invent its own thing and ioctl > isn't enough). Steven? So having the logic in ioctl_fsfreeze(), > freeze_bdev() and freeze_store() in gfs2 seems to be enough. > The sysfs freeze thing is historical and strongly deprecated - I hope that we may be able to remove it one day, Steve. -- 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
On Wed, Feb 18, 2015 at 12:13 PM, Jan Kara <jack@suse.cz> wrote: > On Wed 18-02-15 10:34:55, Alexey Dobriyan wrote: >> On Mon, Feb 16, 2015 at 10:38:52AM +0100, Jan Kara wrote: >> > On Sat 14-02-15 21:55:24, Alexey Dobriyan wrote: >> > > Freezing and thawing are separate system calls, task which is supposed >> > > to thaw filesystem/superblock can disappear due to crash or not thaw >> > > due to a bug. Record at least task name (we can't take task_struct >> > > reference) to make support engineer's life easier. >> > > >> > > Hopefully 16 bytes per superblock isn't much. >> > > >> > > P.S.: Cc'ing GFS2 people just in case they want to correct >> > > my understanding of GFS2 having async freeze code. >> > > >> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> >> > Hum, and when do you show the task name? Or do you expect that customer >> > takes a crashdump and support just finds it in memory? >> >> Yeah, having at least something in crashdump is fine. > OK, then comment about this at freeze_comm[] definition so that it's > clear it isn't just set-but-never-read field. OK. >> > > --- a/fs/ioctl.c >> > > +++ b/fs/ioctl.c >> > > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, >> > > static int ioctl_fsfreeze(struct file *filp) >> > > { >> > > struct super_block *sb = file_inode(filp)->i_sb; >> > > + int rv; >> > > >> > > if (!capable(CAP_SYS_ADMIN)) >> > > return -EPERM; >> > > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) >> > > return -EOPNOTSUPP; >> > > >> > > /* Freeze */ >> > > - if (sb->s_op->freeze_super) >> > > - return sb->s_op->freeze_super(sb); >> > > - return freeze_super(sb); >> > > + if (sb->s_op->freeze_super) { >> > > + rv = sb->s_op->freeze_super(sb); >> > > + if (rv == 0) >> > > + get_task_comm(sb->s_writers.freeze_comm, current); >> > > + } else >> > > + rv = freeze_super(sb); >> > > + return rv; >> > Why don't you just set the name in ioctl_fsfreeze() in both cases? >> >> There are users of freeze_super() in GFS2 unless I'm misreading code. > Yes, there are. The call in fs/gfs2/glops.c is in a call path from > ->freeze_super() handler for GFS2 so that one is handled in > ioctl_fsfreeze() anyway. The call in fs/gfs2/sys.c is a way to freeze > filesystem via sysfs (dunno why GFS2 has to invent its own thing and ioctl > isn't enough). Steven? So having the logic in ioctl_fsfreeze(), > freeze_bdev() and freeze_store() in gfs2 seems to be enough. Jan, my logic is as follows. Recording freezer task name is not filesystem/device specific and thus should be done in generic code. So no changes in GFS2. freeze_super() is generic counterpart to filesystem/device specific ->freeze_super() hook, look how they are paired. It should recore freezer task name, so any future user will not forget to do the same. So it's in ioctl_fsfreeze(), freeze_bdev() and freeze_super(). >> > > --- a/include/linux/fs.h >> > > +++ b/include/linux/fs.h >> > > @@ -1221,6 +1221,8 @@ struct sb_writers { >> > > int frozen; /* Is sb frozen? */ >> > > wait_queue_head_t wait_unfrozen; /* queue for waiting for >> > > sb to be thawed */ >> > > + /* who froze superblock */ >> > > + char freeze_comm[16]; >> > Here should be TASK_COMM_LEN, shouldn't it? >> >> It will pull sched.h, dunno if we care about headers anymore. > That's not ideal but IMHO better than having the value hardcoded here. > That is pretty fragile - i.e. think what happens when someone decides to > increase TASK_COMM_LEN... TASK_COMM_LEN is userspace ABI via at least prctl(PR_SET_NAME). I can formally move it to include/uapi/linux/sched.h. This allows to not drag sched.h into fs.h for one tiny define. Alexey -- 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
On Fri 20-02-15 14:42:29, Alexey Dobriyan wrote: > On Wed, Feb 18, 2015 at 12:13 PM, Jan Kara <jack@suse.cz> wrote: > >> > > --- a/fs/ioctl.c > >> > > +++ b/fs/ioctl.c > >> > > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > >> > > static int ioctl_fsfreeze(struct file *filp) > >> > > { > >> > > struct super_block *sb = file_inode(filp)->i_sb; > >> > > + int rv; > >> > > > >> > > if (!capable(CAP_SYS_ADMIN)) > >> > > return -EPERM; > >> > > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) > >> > > return -EOPNOTSUPP; > >> > > > >> > > /* Freeze */ > >> > > - if (sb->s_op->freeze_super) > >> > > - return sb->s_op->freeze_super(sb); > >> > > - return freeze_super(sb); > >> > > + if (sb->s_op->freeze_super) { > >> > > + rv = sb->s_op->freeze_super(sb); > >> > > + if (rv == 0) > >> > > + get_task_comm(sb->s_writers.freeze_comm, current); > >> > > + } else > >> > > + rv = freeze_super(sb); > >> > > + return rv; > >> > Why don't you just set the name in ioctl_fsfreeze() in both cases? > >> > >> There are users of freeze_super() in GFS2 unless I'm misreading code. > > Yes, there are. The call in fs/gfs2/glops.c is in a call path from > > ->freeze_super() handler for GFS2 so that one is handled in > > ioctl_fsfreeze() anyway. The call in fs/gfs2/sys.c is a way to freeze > > filesystem via sysfs (dunno why GFS2 has to invent its own thing and ioctl > > isn't enough). Steven? So having the logic in ioctl_fsfreeze(), > > freeze_bdev() and freeze_store() in gfs2 seems to be enough. > > Jan, my logic is as follows. > > Recording freezer task name is not filesystem/device specific and > thus should be done in generic code. So no changes in GFS2. > > freeze_super() is generic counterpart to filesystem/device > specific ->freeze_super() hook, look how they are paired. > It should recore freezer task name, so any future user > will not forget to do the same. > > So it's in ioctl_fsfreeze(), freeze_bdev() and freeze_super(). Well, but this stores the name in two different levels - once in the top level (ioctl_fsfreeze(), freeze_bdev()) and once in a place called from there (freeze_super()). That just seems wrong to me. You can just record the frozen process in freeze_super() and mandate that if someone manages to freeze the fs without calling freeze_super() from his ->freeze_super() handler (currently there isn't such filesystem), he is also responsible for setting freezer name... Hmm? > >> > > --- a/include/linux/fs.h > >> > > +++ b/include/linux/fs.h > >> > > @@ -1221,6 +1221,8 @@ struct sb_writers { > >> > > int frozen; /* Is sb frozen? */ > >> > > wait_queue_head_t wait_unfrozen; /* queue for waiting for > >> > > sb to be thawed */ > >> > > + /* who froze superblock */ > >> > > + char freeze_comm[16]; > >> > Here should be TASK_COMM_LEN, shouldn't it? > >> > >> It will pull sched.h, dunno if we care about headers anymore. > > That's not ideal but IMHO better than having the value hardcoded here. > > That is pretty fragile - i.e. think what happens when someone decides to > > increase TASK_COMM_LEN... > > TASK_COMM_LEN is userspace ABI via at least prctl(PR_SET_NAME). > I can formally move it to include/uapi/linux/sched.h. > This allows to not drag sched.h into fs.h for one tiny define. OK, moving the definition would be preferable to me (and IMO also a cleanup). Honza
On Fri, Feb 20, 2015 at 01:15:22PM +0100, Jan Kara wrote: > On Fri 20-02-15 14:42:29, Alexey Dobriyan wrote: > > On Wed, Feb 18, 2015 at 12:13 PM, Jan Kara <jack@suse.cz> wrote: > > >> > > --- a/fs/ioctl.c > > >> > > +++ b/fs/ioctl.c > > >> > > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > > >> > > static int ioctl_fsfreeze(struct file *filp) > > >> > > { > > >> > > struct super_block *sb = file_inode(filp)->i_sb; > > >> > > + int rv; > > >> > > > > >> > > if (!capable(CAP_SYS_ADMIN)) > > >> > > return -EPERM; > > >> > > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) > > >> > > return -EOPNOTSUPP; > > >> > > > > >> > > /* Freeze */ > > >> > > - if (sb->s_op->freeze_super) > > >> > > - return sb->s_op->freeze_super(sb); > > >> > > - return freeze_super(sb); > > >> > > + if (sb->s_op->freeze_super) { > > >> > > + rv = sb->s_op->freeze_super(sb); > > >> > > + if (rv == 0) > > >> > > + get_task_comm(sb->s_writers.freeze_comm, current); > > >> > > + } else > > >> > > + rv = freeze_super(sb); > > >> > > + return rv; > > >> > Why don't you just set the name in ioctl_fsfreeze() in both cases? > > >> > > >> There are users of freeze_super() in GFS2 unless I'm misreading code. > > > Yes, there are. The call in fs/gfs2/glops.c is in a call path from > > > ->freeze_super() handler for GFS2 so that one is handled in > > > ioctl_fsfreeze() anyway. The call in fs/gfs2/sys.c is a way to freeze > > > filesystem via sysfs (dunno why GFS2 has to invent its own thing and ioctl > > > isn't enough). Steven? So having the logic in ioctl_fsfreeze(), > > > freeze_bdev() and freeze_store() in gfs2 seems to be enough. > > > > Jan, my logic is as follows. > > > > Recording freezer task name is not filesystem/device specific and > > thus should be done in generic code. So no changes in GFS2. > > > > freeze_super() is generic counterpart to filesystem/device > > specific ->freeze_super() hook, look how they are paired. > > It should recore freezer task name, so any future user > > will not forget to do the same. > > > > So it's in ioctl_fsfreeze(), freeze_bdev() and freeze_super(). > Well, but this stores the name in two different levels - once in the top > level (ioctl_fsfreeze(), freeze_bdev()) and once in a place called from > there (freeze_super()). That just seems wrong to me. You can just record > the frozen process in freeze_super() and mandate that if someone manages to > freeze the fs without calling freeze_super() from his ->freeze_super() > handler (currently there isn't such filesystem), he is also responsible for > setting freezer name... Hmm? Jan I understand you. But I find stranger that GFS will have to record freezer name. I'm sending v3, hopefully final. > > >> > > --- a/include/linux/fs.h > > >> > > +++ b/include/linux/fs.h > > >> > > @@ -1221,6 +1221,8 @@ struct sb_writers { > > >> > > int frozen; /* Is sb frozen? */ > > >> > > wait_queue_head_t wait_unfrozen; /* queue for waiting for > > >> > > sb to be thawed */ > > >> > > + /* who froze superblock */ > > >> > > + char freeze_comm[16]; > > >> > Here should be TASK_COMM_LEN, shouldn't it? > > >> > > >> It will pull sched.h, dunno if we care about headers anymore. > > > That's not ideal but IMHO better than having the value hardcoded here. > > > That is pretty fragile - i.e. think what happens when someone decides to > > > increase TASK_COMM_LEN... > > > > TASK_COMM_LEN is userspace ABI via at least prctl(PR_SET_NAME). > > I can formally move it to include/uapi/linux/sched.h. > > This allows to not drag sched.h into fs.h for one tiny define. > OK, moving the definition would be preferable to me (and IMO also a > cleanup). -- 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
--- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, static int ioctl_fsfreeze(struct file *filp) { struct super_block *sb = file_inode(filp)->i_sb; + int rv; if (!capable(CAP_SYS_ADMIN)) return -EPERM; @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) return -EOPNOTSUPP; /* Freeze */ - if (sb->s_op->freeze_super) - return sb->s_op->freeze_super(sb); - return freeze_super(sb); + if (sb->s_op->freeze_super) { + rv = sb->s_op->freeze_super(sb); + if (rv == 0) + get_task_comm(sb->s_writers.freeze_comm, current); + } else + rv = freeze_super(sb); + return rv; } static int ioctl_fsthaw(struct file *filp) { struct super_block *sb = file_inode(filp)->i_sb; + int rv; if (!capable(CAP_SYS_ADMIN)) return -EPERM; /* Thaw */ - if (sb->s_op->thaw_super) - return sb->s_op->thaw_super(sb); - return thaw_super(sb); + if (sb->s_op->thaw_super) { + rv = sb->s_op->thaw_super(sb); + if (rv == 0) + memset(sb->s_writers.freeze_comm, 0, TASK_COMM_LEN); + } else + rv = thaw_super(sb); + return rv; } /* --- a/fs/super.c +++ b/fs/super.c @@ -1355,6 +1355,7 @@ int freeze_super(struct super_block *sb) * sees write activity when frozen is set to SB_FREEZE_COMPLETE. */ sb->s_writers.frozen = SB_FREEZE_COMPLETE; + get_task_comm(sb->s_writers.freeze_comm, current); up_write(&sb->s_umount); return 0; } @@ -1391,6 +1392,7 @@ int thaw_super(struct super_block *sb) out: sb->s_writers.frozen = SB_UNFROZEN; + memset(sb->s_writers.freeze_comm, 0, TASK_COMM_LEN); smp_wmb(); wake_up(&sb->s_writers.wait_unfrozen); deactivate_locked_super(sb); --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1221,6 +1221,8 @@ struct sb_writers { int frozen; /* Is sb frozen? */ wait_queue_head_t wait_unfrozen; /* queue for waiting for sb to be thawed */ + /* who froze superblock */ + char freeze_comm[16]; #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map lock_map[SB_FREEZE_LEVELS]; #endif
Freezing and thawing are separate system calls, task which is supposed to thaw filesystem/superblock can disappear due to crash or not thaw due to a bug. Record at least task name (we can't take task_struct reference) to make support engineer's life easier. Hopefully 16 bytes per superblock isn't much. P.S.: Cc'ing GFS2 people just in case they want to correct my understanding of GFS2 having async freeze code. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> --- fs/ioctl.c | 22 ++++++++++++++++------ fs/super.c | 2 ++ include/linux/fs.h | 2 ++ 3 files changed, 20 insertions(+), 6 deletions(-) -- 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