Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752499AbbB1OWn (ORCPT ); Sat, 28 Feb 2015 09:22:43 -0500 Received: from mail-we0-f180.google.com ([74.125.82.180]:44634 "EHLO mail-we0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751461AbbB1OWl (ORCPT ); Sat, 28 Feb 2015 09:22:41 -0500 Date: Sat, 28 Feb 2015 17:22:35 +0300 From: Alexey Dobriyan To: Jan Kara Cc: Andrew Morton , Al Viro , Linux Kernel , linux-fsdevel , swhiteho@redhat.com, cluster-devel@redhat.com Subject: Re: [PATCH] fs: record task name which froze superblock Message-ID: <20150228142235.GA19552@p183.telecom.by> References: <20150214185524.GA16579@p183.telecom.by> <20150216093852.GB4749@quack.suse.cz> <20150218073455.GA1752@p183.telecom.by> <20150218091323.GA4614@quack.suse.cz> <20150220121522.GC6293@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150220121522.GC6293@quack.suse.cz> User-Agent: Mutt/1.5.22 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4131 Lines: 86 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 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-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/