Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754421AbbBTLmc (ORCPT ); Fri, 20 Feb 2015 06:42:32 -0500 Received: from mail-lb0-f177.google.com ([209.85.217.177]:40212 "EHLO mail-lb0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754363AbbBTLmb (ORCPT ); Fri, 20 Feb 2015 06:42:31 -0500 MIME-Version: 1.0 In-Reply-To: <20150218091323.GA4614@quack.suse.cz> References: <20150214185524.GA16579@p183.telecom.by> <20150216093852.GB4749@quack.suse.cz> <20150218073455.GA1752@p183.telecom.by> <20150218091323.GA4614@quack.suse.cz> Date: Fri, 20 Feb 2015 14:42:29 +0300 Message-ID: Subject: Re: [PATCH] fs: record task name which froze superblock From: Alexey Dobriyan To: Jan Kara Cc: Andrew Morton , Al Viro , Linux Kernel , linux-fsdevel , swhiteho@redhat.com, cluster-devel@redhat.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4124 Lines: 95 On Wed, Feb 18, 2015 at 12:13 PM, 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 >> > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/