Date: Wed, 27 Apr 2016 15:38:57 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r298704 - in projects/zfsd/head: cddl/sbin/zfsd lib/libdevdctl lib/libdevdctl/tests Message-ID: <201604271538.u3RFcvNa085390@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Wed Apr 27 15:38:56 2016 New Revision: 298704 URL: https://svnweb.freebsd.org/changeset/base/298704 Log: Make zfsd(8) listen to GEOM::physpath events zfsd should use a newly arrived device for autoreplace by physical path operations, even if that device's physical path isn't known when zfsd first becomes aware of the device. Background: When a new device arrives, devfs emits a CREATE event to devctl(4). In parallel, enc(4) assigns a physical path to it, then geom(4) emits a GEOM::physpath event. It's possible that zfsd will see the CREATE event before the physical path has been assigned. Previously, we would ignore such a device. Now, zfsd(8) will listen to the GEOM::physpath event so it can use the device for autoreplace-by-physical-path actions. lib/libdevctl/event.cc: lib/libdevctl/event.h: lib/libdevctl/tests/libdevctl_unittest.cc Provide GeomEvent objects. Move DevfsEvent::{Devname,IsDiskDev,PhysicalPath,DevPath} into the base Event class. Only DevName needs to be specialized in the derived classes. cddl/sbin/zfsd/zfsd.cc cddl/sbin/zfsd/zfsd_event.cc cddl/sbin/zfsd/zfsd_event.h Listen for GEOM::physpath events. When one arrives, ReEvaluate any matching caseFiles. Sponsored by: Spectra Logic Corp Modified: projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.cc projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.h projects/zfsd/head/lib/libdevdctl/event.cc projects/zfsd/head/lib/libdevdctl/event.h projects/zfsd/head/lib/libdevdctl/tests/libdevdctl_unittest.cc Modified: projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc ============================================================================== --- projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc Wed Apr 27 15:35:05 2016 (r298703) +++ projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc Wed Apr 27 15:38:56 2016 (r298704) @@ -99,6 +99,7 @@ bool ZfsDaemon::s_systemRescanRequ EventFactory::Record ZfsDaemon::s_registryEntries[] = { { Event::NOTIFY, "DEVFS", &DevfsEvent::Builder }, + { Event::NOTIFY, "GEOM", &GeomEvent::Builder }, { Event::NOTIFY, "ZFS", &ZfsEvent::Builder } }; Modified: projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.cc ============================================================================== --- projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.cc Wed Apr 27 15:35:05 2016 (r298703) +++ projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.cc Wed Apr 27 15:38:56 2016 (r298704) @@ -226,6 +226,79 @@ DevfsEvent::DevfsEvent(const DevfsEvent { } +/*-------------------------------- GeomEvent --------------------------------*/ + +//- GeomEvent Static Public Methods ------------------------------------------- +Event * +GeomEvent::Builder(Event::Type type, + NVPairMap &nvPairs, + const string &eventString) +{ + return (new GeomEvent(type, nvPairs, eventString)); +} + +//- GeomEvent Virtual Public Methods ------------------------------------------ +Event * +GeomEvent::DeepCopy() const +{ + return (new GeomEvent(*this)); +} + +bool +GeomEvent::Process() const +{ + /* + * We are only concerned with physical path changes, because those can + * be used to satisfy autoreplace operations + */ + if (Value("type") != "GEOM::physpath" || !IsDiskDev()) + return (false); + + /* Log the event since it is of interest. */ + Log(LOG_INFO); + + string devPath; + if (!DevPath(devPath)) + return (false); + + string physPath; + bool havePhysPath(PhysicalPath(physPath)); + + string devName; + DevName(devName); + + if (havePhysPath) { + /* + * TODO: attempt to resolve events using every casefile + * that matches this physpath + */ + CaseFile *caseFile(CaseFile::Find(physPath)); + if (caseFile != NULL) { + syslog(LOG_INFO, + "Found CaseFile(%s:%s:%s) - ReEvaluating\n", + caseFile->PoolGUIDString().c_str(), + caseFile->VdevGUIDString().c_str(), + zpool_state_to_name(caseFile->VdevState(), + VDEV_AUX_NONE)); + caseFile->ReEvaluate(devPath, physPath, /*vdev*/NULL); + } + } + return (false); +} + +//- GeomEvent Protected Methods ----------------------------------------------- +GeomEvent::GeomEvent(Event::Type type, NVPairMap &nvpairs, + const string &eventString) + : DevdCtl::GeomEvent(type, nvpairs, eventString) +{ +} + +GeomEvent::GeomEvent(const GeomEvent &src) + : DevdCtl::GeomEvent::GeomEvent(src) +{ +} + + /*--------------------------------- ZfsEvent ---------------------------------*/ //- ZfsEvent Static Public Methods --------------------------------------------- DevdCtl::Event * Modified: projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.h ============================================================================== --- projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.h Wed Apr 27 15:35:05 2016 (r298703) +++ projects/zfsd/head/cddl/sbin/zfsd/zfsd_event.h Wed Apr 27 15:38:56 2016 (r298704) @@ -149,4 +149,20 @@ protected: static VdevCallback_t TryDetach; }; +class GeomEvent : public DevdCtl::GeomEvent +{ +public: + static BuildMethod Builder; + + virtual DevdCtl::Event *DeepCopy() const; + + virtual bool Process() const; + +protected: + /** DeepCopy Constructor. */ + GeomEvent(const GeomEvent &src); + + /** Constructor */ + GeomEvent(Type, DevdCtl::NVPairMap &, const string &); +}; #endif /*_ZFSD_EVENT_H_ */ Modified: projects/zfsd/head/lib/libdevdctl/event.cc ============================================================================== --- projects/zfsd/head/lib/libdevdctl/event.cc Wed Apr 27 15:35:05 2016 (r298703) +++ projects/zfsd/head/lib/libdevdctl/event.cc Wed Apr 27 15:38:56 2016 (r298704) @@ -1,5 +1,5 @@ /*- - * Copyright (c) 2011, 2012, 2013 Spectra Logic Corporation + * Copyright (c) 2011, 2012, 2013, 2016 Spectra Logic Corporation * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -69,7 +69,9 @@ __FBSDID("$FreeBSD$"); #define NUM_ELEMENTS(x) (sizeof(x) / sizeof(*x)) /*============================ Namespace Control =============================*/ +using std::begin; using std::cout; +using std::end; using std::endl; using std::string; using std::stringstream; @@ -123,6 +125,50 @@ Event::CreateEvent(const EventFactory &f return (factory.Build(type, nvpairs, eventString)); } +bool +Event::DevName(std::string &name) const +{ + return (false); +} + +/* TODO: simplify this function with C++-11 <regex> methods */ +bool +Event::IsDiskDev() const +{ + static const char *diskDevNames[] = + { + "da", + "ada" + }; + const char **dName; + string devName; + + if (! DevName(devName)) + return false; + + size_t find_start = devName.rfind('/'); + if (find_start == string::npos) { + find_start = 0; + } else { + /* Just after the last '/'. */ + find_start++; + } + + for (dName = begin(diskDevNames); dName <= end(diskDevNames); dName++) { + + size_t loc(devName.find(*dName, find_start)); + if (loc == find_start) { + size_t prefixLen(strlen(*dName)); + + if (devName.length() - find_start >= prefixLen + && isdigit(devName[find_start + prefixLen])) + return (true); + } + } + + return (false); +} + const char * Event::TypeToString(Event::Type type) { @@ -228,6 +274,48 @@ Event::GetTimestamp() const return (tv_timestamp); } +bool +Event::DevPath(std::string &path) const +{ + string devName; + + if (!DevName(devName)) + return (false); + + string devPath(_PATH_DEV + devName); + int devFd(open(devPath.c_str(), O_RDONLY)); + if (devFd == -1) + return (false); + + /* Normalize the device name in case the DEVFS event is for a link. */ + devName = fdevname(devFd); + path = _PATH_DEV + devName; + + close(devFd); + + return (true); +} + +bool +Event::PhysicalPath(std::string &path) const +{ + string devPath; + + if (!DevPath(devPath)) + return (false); + + int devFd(open(devPath.c_str(), O_RDONLY)); + if (devFd == -1) + return (false); + + char physPath[MAXPATHLEN]; + physPath[0] = '\0'; + bool result(ioctl(devFd, DIOCGPHYSPATH, physPath) == 0); + close(devFd); + if (result) + path = physPath; + return (result); +} //- Event Protected Methods ---------------------------------------------------- Event::Event(Type type, NVPairMap &map, const string &eventString) @@ -366,41 +454,6 @@ DevfsEvent::Builder(Event::Type type, NV //- DevfsEvent Static Protected Methods ---------------------------------------- bool -DevfsEvent::IsDiskDev(const string &devName) -{ - static const char *diskDevNames[] = - { - "da", - "ada" - }; - - const char **diskName(diskDevNames); - const char **lastDiskName(diskDevNames - + NUM_ELEMENTS(diskDevNames) - 1); - size_t find_start = devName.rfind('/'); - if (find_start == string::npos) { - find_start = 0; - } else { - /* Just after the last '/'. */ - find_start++; - } - - for (; diskName <= lastDiskName; diskName++) { - - size_t loc(devName.find(*diskName, find_start)); - if (loc == find_start) { - size_t prefixLen(strlen(*diskName)); - - if (devName.length() - find_start >= prefixLen - && isdigit(devName[find_start + prefixLen])) - return (true); - } - } - - return (false); -} - -bool DevfsEvent::IsWholeDev(const string &devName) { string::const_iterator i(devName.begin()); @@ -442,14 +495,6 @@ DevfsEvent::Process() const //- DevfsEvent Public Methods -------------------------------------------------- bool -DevfsEvent::IsDiskDev() const -{ - string devName; - - return (DevName(devName) && IsDiskDev(devName)); -} - -bool DevfsEvent::IsWholeDev() const { string devName; @@ -467,58 +512,53 @@ DevfsEvent::DevName(std::string &name) c return (!name.empty()); } -bool -DevfsEvent::DevPath(std::string &path) const +//- DevfsEvent Protected Methods ----------------------------------------------- +DevfsEvent::DevfsEvent(Event::Type type, NVPairMap &nvpairs, + const string &eventString) + : Event(type, nvpairs, eventString) { - string devName; - - if (!DevName(devName)) - return (false); - - string devPath(_PATH_DEV + devName); - int devFd(open(devPath.c_str(), O_RDONLY)); - if (devFd == -1) - return (false); +} - /* Normalize the device name in case the DEVFS event is for a link. */ - devName = fdevname(devFd); - path = _PATH_DEV + devName; +DevfsEvent::DevfsEvent(const DevfsEvent &src) + : Event(src) +{ +} - close(devFd); +/*--------------------------------- GeomEvent --------------------------------*/ +//- GeomEvent Static Public Methods -------------------------------------------- +Event * +GeomEvent::Builder(Event::Type type, NVPairMap &nvpairs, + const string &eventString) +{ + return (new GeomEvent(type, nvpairs, eventString)); +} - return (true); +//- GeomEvent Virtual Public Methods ------------------------------------------- +Event * +GeomEvent::DeepCopy() const +{ + return (new GeomEvent(*this)); } bool -DevfsEvent::PhysicalPath(std::string &path) const +GeomEvent::DevName(std::string &name) const { - string devPath; - - if (!DevPath(devPath)) - return (false); - - int devFd(open(devPath.c_str(), O_RDONLY)); - if (devFd == -1) - return (false); - - char physPath[MAXPATHLEN]; - physPath[0] = '\0'; - bool result(ioctl(devFd, DIOCGPHYSPATH, physPath) == 0); - close(devFd); - if (result) - path = physPath; - return (result); + name = Value("devname"); + return (!name.empty()); } -//- DevfsEvent Protected Methods ----------------------------------------------- -DevfsEvent::DevfsEvent(Event::Type type, NVPairMap &nvpairs, - const string &eventString) - : Event(type, nvpairs, eventString) + +//- GeomEvent Protected Methods ------------------------------------------------ +GeomEvent::GeomEvent(Event::Type type, NVPairMap &nvpairs, + const string &eventString) + : Event(type, nvpairs, eventString), + m_devname(Value("devname")) { } -DevfsEvent::DevfsEvent(const DevfsEvent &src) - : Event(src) +GeomEvent::GeomEvent(const GeomEvent &src) + : Event(src), + m_devname(src.m_devname) { } @@ -538,6 +578,12 @@ ZfsEvent::DeepCopy() const return (new ZfsEvent(*this)); } +bool +ZfsEvent::DevName(std::string &name) const +{ + return (false); +} + //- ZfsEvent Protected Methods ------------------------------------------------- ZfsEvent::ZfsEvent(Event::Type type, NVPairMap &nvpairs, const string &eventString) Modified: projects/zfsd/head/lib/libdevdctl/event.h ============================================================================== --- projects/zfsd/head/lib/libdevdctl/event.h Wed Apr 27 15:35:05 2016 (r298703) +++ projects/zfsd/head/lib/libdevdctl/event.h Wed Apr 27 15:38:56 2016 (r298704) @@ -1,5 +1,5 @@ /*- - * Copyright (c) 2011, 2012, 2013 Spectra Logic Corporation + * Copyright (c) 2011, 2012, 2013, 2016 Spectra Logic Corporation * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -101,6 +101,36 @@ public: const std::string &eventString); /** + * Returns the devname, if any, associated with the event + * + * \param name Devname, returned by reference + * \return True iff the event contained a devname + */ + virtual bool DevName(std::string &name) const; + + /** + * Returns the absolute pathname of the device associated with this + * event. + * + * \param name Devname, returned by reference + * \return True iff the event contained a devname + */ + bool DevPath(std::string &path) const; + + /** + * Returns true iff this event refers to a disk device + */ + bool IsDiskDev() const; + + /** Returns the physical path of the device, if any + * + * \param path Physical path, returned by reference + * \return True iff the event contains a device with a physical + * path + */ + bool PhysicalPath(std::string &path) const; + + /** * Provide a user friendly string representation of an * event type. * @@ -298,22 +328,11 @@ public: */ virtual bool Process() const; - bool IsDiskDev() const; bool IsWholeDev() const; - bool DevName(std::string &name) const; - bool DevPath(std::string &path) const; - bool PhysicalPath(std::string &path) const; + virtual bool DevName(std::string &name) const; protected: /** - * Determine if the given device name references a potential - * disk device. - * - * \param devName The device name to test. - */ - static bool IsDiskDev(const std::string &devName); - - /** * Given the device name of a disk, determine if the device * represents the whole device, not just a partition. * @@ -331,6 +350,29 @@ protected: DevfsEvent(Type, NVPairMap &, const std::string &); }; +/*--------------------------------- GeomEvent --------------------------------*/ +class GeomEvent : public Event +{ +public: + /** Specialized Event object factory for GEOM events. */ + static BuildMethod Builder; + + virtual Event *DeepCopy() const; + + virtual bool DevName(std::string &name) const; + + const std::string &DeviceName() const; + +protected: + /** Constructor */ + GeomEvent(Type, NVPairMap &, const std::string &); + + /** Deep copy constructor. */ + GeomEvent(const GeomEvent &src); + + std::string m_devname; +}; + /*--------------------------------- ZfsEvent ---------------------------------*/ class ZfsEvent : public Event { @@ -340,6 +382,8 @@ public: virtual Event *DeepCopy() const; + virtual bool DevName(std::string &name) const; + const std::string &PoolName() const; Guid PoolGUID() const; Guid VdevGUID() const; Modified: projects/zfsd/head/lib/libdevdctl/tests/libdevdctl_unittest.cc ============================================================================== --- projects/zfsd/head/lib/libdevdctl/tests/libdevdctl_unittest.cc Wed Apr 27 15:35:05 2016 (r298703) +++ projects/zfsd/head/lib/libdevdctl/tests/libdevdctl_unittest.cc Wed Apr 27 15:38:56 2016 (r298704) @@ -44,13 +44,21 @@ using namespace DevdCtl; using namespace std; using namespace testing; -#define NUM_ELEMENTS(x) (sizeof(x) / sizeof(*x)) +#define REGISTRY_SIZE 2 -class IsDiskDevTest : public TestWithParam<pair<bool, const char*> >{ +struct DevNameTestParams +{ + const char* evs; + bool is_disk; + const char* devname; +}; + +class DevNameTest : public TestWithParam<DevNameTestParams>{ protected: virtual void SetUp() { m_factory = new EventFactory(); + m_factory->UpdateRegistry(s_registry, REGISTRY_SIZE); } virtual void TearDown() @@ -61,53 +69,68 @@ protected: EventFactory *m_factory; Event *m_ev; - static EventFactory::Record s_registry[]; + static EventFactory::Record s_registry[REGISTRY_SIZE]; }; -DevdCtl::EventFactory::Record IsDiskDevTest::s_registry[] = { - { Event::NOTIFY, "DEVFS", &DevfsEvent::Builder } +DevdCtl::EventFactory::Record DevNameTest::s_registry[REGISTRY_SIZE] = { + { Event::NOTIFY, "DEVFS", &DevfsEvent::Builder }, + { Event::NOTIFY, "GEOM", &GeomEvent::Builder } }; -TEST_P(IsDiskDevTest, TestIsDiskDev) { - pair<bool, const char*> param = GetParam(); - DevfsEvent *devfs_ev; +TEST_P(DevNameTest, TestDevname) { + std::string devname; + DevNameTestParams param = GetParam(); + + string evString(param.evs); + m_ev = Event::CreateEvent(*m_factory, evString); + m_ev->DevName(devname); + EXPECT_STREQ(param.devname, devname.c_str()); +} + +TEST_P(DevNameTest, TestIsDiskDev) { + DevNameTestParams param = GetParam(); - m_factory->UpdateRegistry(s_registry, NUM_ELEMENTS(s_registry)); - string evString(param.second); + string evString(param.evs); m_ev = Event::CreateEvent(*m_factory, evString); - devfs_ev = dynamic_cast<DevfsEvent*>(m_ev); - ASSERT_NE(nullptr, devfs_ev); - EXPECT_EQ(param.first, devfs_ev->IsDiskDev()); + EXPECT_EQ(param.is_disk, m_ev->IsDiskDev()); } -INSTANTIATE_TEST_CASE_P(IsDiskDevTestInstantiation, IsDiskDevTest, Values( - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=da6\n"), - pair<bool, const char*>(false, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=cuau0\n"), - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=ada6\n"), - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=da6p1\n"), - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=ada6p1\n"), - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=da6s0p1\n"), - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=ada6s0p1\n"), +/* TODO: clean this up using C++-11 uniform initializers */ +INSTANTIATE_TEST_CASE_P(IsDiskDevTestInstantiation, DevNameTest, Values( + (DevNameTestParams){ + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=da6\n", + .is_disk = true, .devname = "da6"}, + (DevNameTestParams){.is_disk = false, .devname = "cuau0", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=cuau0\n"}, + (DevNameTestParams){.is_disk = true, .devname = "ada6", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=ada6\n"}, + (DevNameTestParams){.is_disk = true, .devname = "da6p1", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=da6p1\n"}, + (DevNameTestParams){.is_disk = true, .devname = "ada6p1", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=ada6p1\n"}, + (DevNameTestParams){.is_disk = true, .devname = "da6s0p1", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=da6s0p1\n"}, + (DevNameTestParams){.is_disk = true, .devname = "ada6s0p1", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=ada6s0p1\n"}, /* * Test physical path nodes. These are currently all set to false since * physical path nodes are implemented with symlinks, and most CAM and * ZFS operations can't use symlinked device nodes */ /* A SpectraBSD-style physical path node*/ - pair<bool, const char*>(false, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@50030480019f53fd/elmtype@array_device/slot@18/da\n"), - pair<bool, const char*>(false, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@50030480019f53fd/elmtype@array_device/slot@18/pass\n"), + (DevNameTestParams){.is_disk = false, .devname = "enc@50030480019f53fd/elmtype@array_device/slot@18/da", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@50030480019f53fd/elmtype@array_device/slot@18/da\n"}, + (DevNameTestParams){.is_disk = false, .devname = "enc@50030480019f53fd/elmtype@array_device/slot@18/pass", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@50030480019f53fd/elmtype@array_device/slot@18/pass\n"}, /* A FreeBSD-style physical path node */ - pair<bool, const char*>(true, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@n50030480019f53fd/type@0/slot@18/elmdesc@ArrayDevice18/da6\n"), - pair<bool, const char*>(false, - "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@n50030480019f53fd/type@0/slot@18/elmdesc@ArrayDevice18/pass6\n")); + (DevNameTestParams){.is_disk = true, .devname = "enc@n50030480019f53fd/type@0/slot@18/elmdesc@ArrayDevice18/da6", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@n50030480019f53fd/type@0/slot@18/elmdesc@ArrayDevice18/da6\n"}, + (DevNameTestParams){.is_disk = false, .devname = "enc@n50030480019f53fd/type@0/slot@18/elmdesc@ArrayDevice18/pass6", + .evs = "!system=DEVFS subsystem=CDEV type=CREATE cdev=enc@n50030480019f53fd/type@0/slot@18/elmdesc@ArrayDevice18/pass6\n"}, + + /* + * Test some GEOM events + */ + (DevNameTestParams){.is_disk = true, .devname = "da5", + .evs = "!system=GEOM subsystem=disk type=GEOM::physpath devname=da5\n"}) );
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201604271538.u3RFcvNa085390>