From: Akinobu Mita <akinobu.mita@gmail.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-nvme@lists.infradead.org,
LKML <linux-kernel@vger.kernel.org>,
Keith Busch <keith.busch@intel.com>, Jens Axboe <axboe@fb.com>,
Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>
Subject: Re: [PATCH 2/4] devcoredump: allow to create several coredump files in one device
Date: Fri, 3 May 2019 12:41:36 +0900 [thread overview]
Message-ID: <CAC5umyhyVNA63OUQsw=SSP_poPOwQ+Y7sPRRpGLaJXb7T-C3Ug@mail.gmail.com> (raw)
In-Reply-To: <f0f772e5e33519dac93672be26fa7995f8109721.camel@sipsolutions.net>
2019年5月2日(木) 21:47 Johannes Berg <johannes@sipsolutions.net>:
>
> On Thu, 2019-05-02 at 17:59 +0900, Akinobu Mita wrote:
> >
> > static void devcd_del(struct work_struct *wk)
> > {
> > struct devcd_entry *devcd;
> > + int i;
> >
> > devcd = container_of(wk, struct devcd_entry, del_wk.work);
> >
> > + for (i = 0; i < devcd->num_files; i++) {
> > + device_remove_bin_file(&devcd->devcd_dev,
> > + &devcd->files[i].bin_attr);
> > + }
>
> Not much value in the braces?
OK. I tend to use braces where a single statement but multiple lines.
> > +static struct devcd_entry *devcd_alloc(struct dev_coredumpm_bulk_data *files,
> > + int num_files, gfp_t gfp)
> > +{
> > + struct devcd_entry *devcd;
> > + int i;
> > +
> > + devcd = kzalloc(sizeof(*devcd), gfp);
> > + if (!devcd)
> > + return NULL;
> > +
> > + devcd->files = kcalloc(num_files, sizeof(devcd->files[0]), gfp);
> > + if (!devcd->files) {
> > + kfree(devcd);
> > + return NULL;
> > + }
> > + devcd->num_files = num_files;
>
> IMHO it would be nicer to allocate all of this in one struct, i.e. have
>
> struct devcd_entry {
> ...
> struct devcd_file files[];
> }
>
> (and then use struct_size())
Sounds good.
> > @@ -309,7 +339,41 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> > put_module:
> > module_put(owner);
> > free:
> > - free(data);
> > + for (i = 0; i < num_files; i++)
> > + files[i].free(files[i].data);
> > +}
>
> and then you don't need to do all this kind of thing to free
>
> Otherwise looks fine. I'd worry a bit that existing userspace will only
> capture the 'data' file, rather than a tarball of all files, but I guess
> that's something you'd have to work out then when actually desiring to
> use multiple files.
Your worrying is correct. I'm going to create a empty 'data' file for nvme
coredump. Assuming that devcd* always contains the 'data' file at least,
we can simply write to 'data' when the device coredump is no longer needed,
and prepare for the newer coredump.
next prev parent reply other threads:[~2019-05-03 3:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-02 8:59 [PATCH 0/4] nvme-pci: support device coredump Akinobu Mita
2019-05-02 8:59 ` [PATCH 1/4] devcoredump: use memory_read_from_buffer Akinobu Mita
2019-05-02 12:42 ` Johannes Berg
2019-05-02 8:59 ` [PATCH 2/4] devcoredump: allow to create several coredump files in one device Akinobu Mita
2019-05-02 12:47 ` Johannes Berg
2019-05-03 3:41 ` Akinobu Mita [this message]
2019-05-02 8:59 ` [PATCH 3/4] nvme-pci: add device coredump support Akinobu Mita
2019-05-04 10:04 ` Minwoo Im
2019-05-04 14:26 ` Akinobu Mita
2019-05-04 14:38 ` Minwoo Im
2019-05-04 14:46 ` Minwoo Im
2019-05-02 8:59 ` [PATCH 4/4] nvme-pci: trigger device coredump before resetting controller Akinobu Mita
2019-05-02 12:57 ` [PATCH 0/4] nvme-pci: support device coredump Keith Busch
2019-05-03 3:38 ` Akinobu Mita
2019-05-03 12:12 ` Keith Busch
2019-05-03 12:20 ` Christoph Hellwig
2019-05-04 4:20 ` Akinobu Mita
2019-05-04 9:40 ` Minwoo Im
2019-05-04 14:36 ` Akinobu Mita
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAC5umyhyVNA63OUQsw=SSP_poPOwQ+Y7sPRRpGLaJXb7T-C3Ug@mail.gmail.com' \
--to=akinobu.mita@gmail.com \
--cc=axboe@fb.com \
--cc=hch@lst.de \
--cc=johannes@sipsolutions.net \
--cc=keith.busch@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=sagi@grimberg.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).