Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
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 j...@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 */ + charfreeze_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).
Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
On Wed, Feb 18, 2015 at 12:13 PM, Jan Kara j...@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 adobri...@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 */ + charfreeze_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
Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
On Fri 20-02-15 14:42:29, Alexey Dobriyan wrote: On Wed, Feb 18, 2015 at 12:13 PM, Jan Kara j...@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 */ + charfreeze_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 -- Jan Kara j...@suse.cz SUSE Labs, CR
Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
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 adobri...@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 */ + charfreeze_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 -- Jan Kara j...@suse.cz SUSE Labs, CR
Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
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 adobri...@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.
Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
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 adobri...@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 */ + charfreeze_comm[16]; Here should be TASK_COMM_LEN, shouldn't it? It will pull sched.h, dunno if we care about headers anymore. Alexey
Re: [Cluster-devel] [PATCH] fs: record task name which froze superblock
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 adobri...@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 */ + charfreeze_comm[16]; Here should be TASK_COMM_LEN, shouldn't it? Honza -- Jan Kara j...@suse.cz SUSE Labs, CR